Skip to content

Commit 51e1bbd

Browse files
Rumata888ludamad
authored andcommitted
fix: BB acir expression conversion issue
Fixes a bug discovered by Consensys Dilligence in expression conversion
1 parent 28719b1 commit 51e1bbd

File tree

3 files changed

+89
-8
lines changed

3 files changed

+89
-8
lines changed

barretenberg/cpp/scripts/test_civc_standalone_vks_havent_changed.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ cd ..
1414
# - Upload the compressed results: aws s3 cp bb-civc-inputs.tar.gz s3://aztec-ci-artifacts/protocol/bb-civc-inputs-[hash(0:8)].tar.gz
1515
# Note: In case of the "Test suite failed to run ... Unexpected token 'with' " error, need to run: docker pull aztecprotocol/build:3.0
1616

17-
pinned_short_hash="ec9b5be3"
17+
pinned_short_hash="e49e8d04"
1818
pinned_civc_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-civc-inputs-${pinned_short_hash}.tar.gz"
1919

2020
function compress_and_upload {

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include "acir_format.hpp"
66
#include "acir_format_mocks.hpp"
7+
#include "acir_to_constraint_buf.hpp"
78
#include "barretenberg/common/streams.hpp"
89
#include "barretenberg/op_queue/ecc_op_queue.hpp"
910

@@ -341,3 +342,82 @@ TEST_F(AcirFormatTests, TestBigAdd)
341342

342343
EXPECT_TRUE(CircuitChecker::check(builder));
343344
}
345+
346+
// Helper function to convert a uint256_t to a 32-byte vector in big-endian format
347+
std::vector<uint8_t> to_bytes_be(uint256_t value)
348+
{
349+
std::vector<uint8_t> bytes(32, 0);
350+
for (size_t i = 0; i < 32; i++) {
351+
bytes[31 - i] = static_cast<uint8_t>(value & 0xFF);
352+
value >>= 8;
353+
}
354+
return bytes;
355+
}
356+
357+
/**
358+
* @brief Test for bug fix where expressions with distinct witnesses requiring more than one width-4 gate
359+
* were incorrectly processed when they initially appeared to fit in width-3 gates
360+
*
361+
* @details This test verifies the fix for a bug in handle_arithmetic where an expression with:
362+
* - 1 mul term using witnesses (w0 * w1)
363+
* - 3 additional linear terms using distinct witnesses (w2, w3, w4)
364+
*
365+
* Such expressions have ≤3 linear combinations and ≤1 mul term, appearing to fit in a
366+
* poly_triple (width-3) gate. However, with all 5 witnesses distinct, serialize_arithmetic_gate
367+
* correctly returns all zeros, indicating it cannot fit in a width-3 gate.
368+
*
369+
* The bug: old code would check if poly_triple was all zeros, and if so, directly add to
370+
* quad_constraints via serialize_mul_quad_gate. But it did this inside the initial
371+
* might_fit_in_polytriple check, so it would never properly go through the mul_quad processing
372+
* logic that handles the general case with >4 witnesses.
373+
*
374+
* The fix: now uses a needs_to_be_parsed_as_mul_quad flag that is set when poly_triple fails,
375+
* and processes through the proper mul_quad logic path, which splits into multiple gates.
376+
*
377+
* Expression: w0 * w1 + w2 + w3 + w4 = 10
378+
* With witnesses: w0=0, w1=1, w2=2, w3=3, w4=4
379+
* Evaluation: 0*1 + 2 + 3 + 4 = 9, but we set q_c = -9, so constraint is: 9 - 9 = 0
380+
*/
381+
TEST_F(AcirFormatTests, TestArithmeticGateWithDistinctWitnessesRegression)
382+
{
383+
// Create an ACIR expression: w0 * w1 + w2 + w3 + w4 - 9 = 0
384+
// This has 1 mul term and 3 linear terms with all 5 distinct witnesses (requires multiple width-4 gates)
385+
Acir::Expression expr{ .mul_terms = { std::make_tuple(
386+
to_bytes_be(1), Acir::Witness{ .value = 0 }, Acir::Witness{ .value = 1 }) },
387+
.linear_combinations = { std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 2 }),
388+
std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 3 }),
389+
std::make_tuple(to_bytes_be(1), Acir::Witness{ .value = 4 }) },
390+
.q_c = to_bytes_be(static_cast<uint256_t>(fr(-9))) };
391+
392+
Acir::Opcode::AssertZero assert_zero{ .value = expr };
393+
394+
// Create an ACIR circuit with this opcode
395+
Acir::Circuit circuit{
396+
.current_witness_index = 5,
397+
.opcodes = { Acir::Opcode{ .value = assert_zero } },
398+
.return_values = {},
399+
};
400+
401+
Acir::Program program{ .functions = { circuit } };
402+
403+
// Serialize the program to bytes
404+
auto program_bytes = program.bincodeSerialize();
405+
406+
// Process through circuit_buf_to_acir_format (this calls handle_arithmetic internally)
407+
AcirFormat constraint_system = circuit_buf_to_acir_format(std::move(program_bytes));
408+
409+
// The key assertion: this expression should end up in big_quad_constraints, not poly_triple_constraints
410+
// or single quad_constraints, because it needs 5 witness slots (all distinct)
411+
EXPECT_EQ(constraint_system.poly_triple_constraints.size(), 0);
412+
EXPECT_EQ(constraint_system.quad_constraints.size(), 0);
413+
EXPECT_EQ(constraint_system.big_quad_constraints.size(), 1);
414+
415+
// Now verify the constraint system with valid witness assignments
416+
// We need: w0 * w1 + w2 + w3 + w4 = 9
417+
// Use values: w0=0, w1=1, w2=2, w3=3, w4=4, so 0*1 + 2 + 3 + 4 = 9
418+
WitnessVector witness{ 0, 1, 2, 3, 4 };
419+
AcirProgram acir_program{ constraint_system, witness };
420+
auto builder = create_circuit(acir_program);
421+
422+
EXPECT_TRUE(CircuitChecker::check(builder));
423+
}

barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.cpp

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,9 @@ std::pair<uint32_t, uint32_t> is_assert_equal(Acir::Opcode::AssertZero const& ar
459459
void handle_arithmetic(Acir::Opcode::AssertZero const& arg, AcirFormat& af, size_t opcode_index)
460460
{
461461
// If the expression fits in a polytriple, we use it.
462-
if (arg.value.linear_combinations.size() <= 3 && arg.value.mul_terms.size() <= 1) {
462+
bool might_fit_in_polytriple = arg.value.linear_combinations.size() <= 3 && arg.value.mul_terms.size() <= 1;
463+
bool needs_to_be_parsed_as_mul_quad = !might_fit_in_polytriple;
464+
if (might_fit_in_polytriple) {
463465
poly_triple pt = serialize_arithmetic_gate(arg.value);
464466

465467
auto assert_equal = is_assert_equal(arg, pt, af);
@@ -502,15 +504,14 @@ void handle_arithmetic(Acir::Opcode::AssertZero const& arg, AcirFormat& af, size
502504
// gate. This is the case if the linear terms are all distinct witness from the multiplication term. In that
503505
// case, the serialize_arithmetic_gate() function will return a poly_triple with all 0's, and we use a width-4
504506
// gate instead. We could probably always use a width-4 gate in fact.
505-
if (pt == poly_triple{ 0, 0, 0, 0, 0, 0, 0, 0 }) {
506-
af.quad_constraints.push_back(serialize_mul_quad_gate(arg.value));
507-
af.original_opcode_indices.quad_constraints.push_back(opcode_index);
508-
509-
} else {
507+
if (pt != poly_triple{ 0, 0, 0, 0, 0, 0, 0, 0 }) {
510508
af.poly_triple_constraints.push_back(pt);
511509
af.original_opcode_indices.poly_triple_constraints.push_back(opcode_index);
510+
} else {
511+
needs_to_be_parsed_as_mul_quad = true;
512512
}
513-
} else {
513+
}
514+
if (needs_to_be_parsed_as_mul_quad) {
514515
std::vector<mul_quad_<fr>> mul_quads;
515516
// We try to use a single mul_quad gate to represent the expression.
516517
if (arg.value.mul_terms.size() <= 1) {

0 commit comments

Comments
 (0)