fix: Start witness of ACIR generated by Noir start at zero not one (#…

…3961) Resolves noir-lang/noir#130 Only partially resovles AztecProtocol/barretenberg#816. There is some places like in `serialize_arithmetic_gate` where we assume that the `zero_idx == 0`. --------- Co-authored-by: ledwards2225 <[email protected]> Co-authored-by: ledwards2225 <[email protected]> Co-authored-by: Tom French <[email protected]> Co-authored-by: kevaundray <[email protected]>
AztecProtocol · Jan 16, 2024 · 4cdc096 · 4cdc096
1 parent fd58251
commit 4cdc096
Show file tree

Hide file tree

Showing 15 changed files with 106 additions and 126 deletions.
diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_format.test.cpp
@@ -79,24 +79,24 @@ TEST_F(AcirFormatTests, TestLogicGateFromNoirCircuit)
      * }
      **/
     RangeConstraint range_a{
-        .witness = 1,
+        .witness = 0,
         .num_bits = 32,
     };
     RangeConstraint range_b{
-        .witness = 2,
+        .witness = 1,
         .num_bits = 32,
     };
 
     LogicConstraint logic_constraint{
-        .a = 1,
-        .b = 2,
-        .result = 3,
+        .a = 0,
+        .b = 1,
+        .result = 2,
         .num_bits = 32,
         .is_xor_gate = 1,
     };
     poly_triple expr_a{
-        .a = 3,
-        .b = 4,
+        .a = 2,
+        .b = 3,
         .c = 0,
         .q_m = 0,
         .q_l = 1,
@@ -105,19 +105,19 @@ TEST_F(AcirFormatTests, TestLogicGateFromNoirCircuit)
         .q_c = -10,
     };
     poly_triple expr_b{
-        .a = 4,
-        .b = 5,
-        .c = 6,
+        .a = 3,
+        .b = 4,
+        .c = 5,
         .q_m = 1,
         .q_l = 0,
         .q_r = 0,
         .q_o = -1,
         .q_c = 0,
     };
     poly_triple expr_c{
-        .a = 4,
-        .b = 6,
-        .c = 4,
+        .a = 3,
+        .b = 5,
+        .c = 3,
         .q_m = 1,
         .q_l = 0,
         .q_r = 0,
@@ -126,7 +126,7 @@ TEST_F(AcirFormatTests, TestLogicGateFromNoirCircuit)
 
     };
     poly_triple expr_d{
-        .a = 6,
+        .a = 5,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -139,8 +139,8 @@ TEST_F(AcirFormatTests, TestLogicGateFromNoirCircuit)
     // EXPR [ (1, _4, _6) (-1, _4) 0 ]
     // EXPR [ (-1, _6) 1 ]
 
-    acir_format constraint_system{ .varnum = 7,
-                                   .public_inputs = { 2 },
+    acir_format constraint_system{ .varnum = 6,
+                                   .public_inputs = { 1 },
                                    .logic_constraints = { logic_constraint },
                                    .range_constraints = { range_a, range_b },
                                    .sha256_constraints = {},
@@ -181,13 +181,13 @@ TEST_F(AcirFormatTests, TestSchnorrVerifyPass)
     std::vector<RangeConstraint> range_constraints;
     for (uint32_t i = 0; i < 10; i++) {
         range_constraints.push_back(RangeConstraint{
-            .witness = i + 1,
+            .witness = i,
             .num_bits = 15,
         });
     }
 
     std::vector<uint32_t> signature(64);
-    for (uint32_t i = 0, value = 13; i < 64; i++, value++) {
+    for (uint32_t i = 0, value = 12; i < 64; i++, value++) {
         signature[i] = value;
         range_constraints.push_back(RangeConstraint{
             .witness = value,
@@ -196,13 +196,13 @@ TEST_F(AcirFormatTests, TestSchnorrVerifyPass)
     }
 
     SchnorrConstraint schnorr_constraint{
-        .message = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 },
-        .public_key_x = 11,
-        .public_key_y = 12,
-        .result = 77,
+        .message = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 },
+        .public_key_x = 10,
+        .public_key_y = 11,
+        .result = 76,
         .signature = signature,
     };
-    acir_format constraint_system{ .varnum = 82,
+    acir_format constraint_system{ .varnum = 81,
                                    .public_inputs = {},
                                    .logic_constraints = {},
                                    .range_constraints = range_constraints,
@@ -271,13 +271,13 @@ TEST_F(AcirFormatTests, TestSchnorrVerifySmallRange)
     std::vector<RangeConstraint> range_constraints;
     for (uint32_t i = 0; i < 10; i++) {
         range_constraints.push_back(RangeConstraint{
-            .witness = i + 1,
+            .witness = i,
             .num_bits = 8,
         });
     }
 
     std::vector<uint32_t> signature(64);
-    for (uint32_t i = 0, value = 13; i < 64; i++, value++) {
+    for (uint32_t i = 0, value = 12; i < 64; i++, value++) {
         signature[i] = value;
         range_constraints.push_back(RangeConstraint{
             .witness = value,
@@ -286,14 +286,14 @@ TEST_F(AcirFormatTests, TestSchnorrVerifySmallRange)
     }
 
     SchnorrConstraint schnorr_constraint{
-        .message = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 },
-        .public_key_x = 11,
-        .public_key_y = 12,
-        .result = 77,
+        .message = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 },
+        .public_key_x = 10,
+        .public_key_y = 11,
+        .result = 76,
         .signature = signature,
     };
     acir_format constraint_system{
-        .varnum = 82,
+        .varnum = 81,
         .public_inputs = {},
         .logic_constraints = {},
         .range_constraints = range_constraints,
@@ -360,39 +360,39 @@ TEST_F(AcirFormatTests, TestSchnorrVerifySmallRange)
 TEST_F(AcirFormatTests, TestVarKeccak)
 {
     HashInput input1;
-    input1.witness = 1;
+    input1.witness = 0;
     input1.num_bits = 8;
     HashInput input2;
-    input2.witness = 2;
+    input2.witness = 1;
     input2.num_bits = 8;
     HashInput input3;
-    input3.witness = 3;
+    input3.witness = 2;
     input3.num_bits = 8;
     KeccakVarConstraint keccak;
     keccak.inputs = { input1, input2, input3 };
-    keccak.var_message_size = 4;
-    keccak.result = { 5,  6,  7,  8,  9,  10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20,
-                      21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36 };
+    keccak.var_message_size = 3;
+    keccak.result = { 4,  5,  6,  7,  8,  9,  10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+                      20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35 };
 
     RangeConstraint range_a{
-        .witness = 1,
+        .witness = 0,
         .num_bits = 8,
     };
     RangeConstraint range_b{
-        .witness = 2,
+        .witness = 1,
         .num_bits = 8,
     };
     RangeConstraint range_c{
-        .witness = 3,
+        .witness = 2,
         .num_bits = 8,
     };
     RangeConstraint range_d{
-        .witness = 4,
+        .witness = 3,
         .num_bits = 8,
     };
 
     auto dummy = poly_triple{
-        .a = 1,
+        .a = 0,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -403,7 +403,7 @@ TEST_F(AcirFormatTests, TestVarKeccak)
     };
 
     acir_format constraint_system{
-        .varnum = 37,
+        .varnum = 36,
         .public_inputs = {},
         .logic_constraints = {},
         .range_constraints = { range_a, range_b, range_c, range_d },

diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.hpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/acir_to_constraint_buf.hpp
@@ -29,12 +29,12 @@ namespace acir_format {
  */
 poly_triple serialize_arithmetic_gate(Circuit::Expression const& arg)
 {
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): Instead of zeros for a,b,c, what we really want
-    // is something like the bberg zero_idx. Hardcoding these to 0 has the same effect only if zero_idx == 0, as has
-    // historically been the case. If that changes, this might break since now "0" points to some non-zero witness in
-    // variables. (From some testing, it seems like it may not break but only because the selectors multiplying the
-    // erroneously non-zero value are zero. Still, it seems like a bad idea to have erroneous wire values even if they
-    // dont break the relation. They'll still add cost in commitments, for example).
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): The initialization of the witness indices a,b,c
+    // to 0 is implicitly assuming that (builder.zero_idx == 0) which is no longer the case. Now, witness idx 0 in
+    // general will correspond to some non-zero value and some witnesses which are not explicitly set below will be
+    // erroneously populated with this value. This does not cause failures however because the corresponding selector
+    // will indeed be 0 so the gate will be satisfied. Still, its a bad idea to have erroneous wire values
+    // even if they dont break the relation. They'll still add cost in commitments, for example.
     poly_triple pt{
         .a = 0,
         .b = 0,
@@ -72,8 +72,8 @@ poly_triple serialize_arithmetic_gate(Circuit::Expression const& arg)
         // If the witness index has not yet been set or if the corresponding linear term is active, set the witness
         // index and the corresponding selector value.
         // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): May need to adjust the pt.a == witness_idx
-        // check (and the others like it) since we initialize a,b,c with 0 but 0 becomes a valid witness index if the +1
-        // offset is removed from noir.
+        // check (and the others like it) since we initialize a,b,c with 0 but 0 is a valid witness index once the
+        // +1 offset is removed from noir.
         if (!a_set || pt.a == witness_idx) { // q_l * w_l
             pt.a = witness_idx;
             pt.q_l = selector_value;
@@ -294,7 +294,8 @@ acir_format circuit_buf_to_acir_format(std::vector<uint8_t> const& buf)
     auto circuit = Circuit::Circuit::bincodeDeserialize(buf);
 
     acir_format af;
-    af.varnum = circuit.current_witness_index;
+    // `varnum` is the true number of variables, thus we add one to the index which starts at zero
+    af.varnum = circuit.current_witness_index + 1;
     af.public_inputs = join({ map(circuit.public_parameters.value, [](auto e) { return e.value; }),
                               map(circuit.return_values.value, [](auto e) { return e.value; }) });
     std::map<uint32_t, BlockConstraint> block_id_to_block_constraint;
@@ -340,9 +341,7 @@ WitnessVector witness_buf_to_witness_data(std::vector<uint8_t> const& buf)
 {
     auto w = WitnessMap::WitnessMap::bincodeDeserialize(buf);
     WitnessVector wv;
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/816): Does "index" need to be initialized to 0 once we
-    // get rid of the +1 offset in noir?
-    size_t index = 1;
+    size_t index = 0;
     for (auto& e : w.value) {
         // ACIR uses a sparse format for WitnessMap where unused witness indices may be left unassigned.
         // To ensure that witnesses sit at the correct indices in the `WitnessVector`, we fill any indices

diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/block_constraint.test.cpp
@@ -14,13 +14,13 @@ class UltraPlonkRAM : public ::testing::Test {
 };
 size_t generate_block_constraint(BlockConstraint& constraint, WitnessVector& witness_values)
 {
-    size_t witness_len = 1;
+    size_t witness_len = 0;
     witness_values.emplace_back(1);
     witness_len++;
 
     fr two = fr::one() + fr::one();
     poly_triple a0 = poly_triple{
-        .a = 1,
+        .a = 0,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -41,7 +41,7 @@ size_t generate_block_constraint(BlockConstraint& constraint, WitnessVector& wit
         .q_c = three,
     };
     poly_triple r1 = poly_triple{
-        .a = 1,
+        .a = 0,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -51,7 +51,7 @@ size_t generate_block_constraint(BlockConstraint& constraint, WitnessVector& wit
         .q_c = fr::neg_one(),
     };
     poly_triple r2 = poly_triple{
-        .a = 1,
+        .a = 0,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -61,7 +61,7 @@ size_t generate_block_constraint(BlockConstraint& constraint, WitnessVector& wit
         .q_c = fr::neg_one(),
     };
     poly_triple y = poly_triple{
-        .a = 2,
+        .a = 1,
         .b = 0,
         .c = 0,
         .q_m = 0,
@@ -73,7 +73,7 @@ size_t generate_block_constraint(BlockConstraint& constraint, WitnessVector& wit
     witness_values.emplace_back(2);
     witness_len++;
     poly_triple z = poly_triple{
-        .a = 3,
+        .a = 2,
         .b = 0,
         .c = 0,
         .q_m = 0,

diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_secp256k1.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_secp256k1.test.cpp
@@ -40,7 +40,7 @@ size_t generate_ecdsa_constraint(EcdsaSecp256k1Constraint& ecdsa_constraint, Wit
     std::vector<uint32_t> pub_x_indices_in;
     std::vector<uint32_t> pub_y_indices_in;
     std::vector<uint32_t> signature_in;
-    size_t offset = 1;
+    size_t offset = 0;
     for (size_t i = 0; i < hashed_message.size(); ++i) {
         message_in.emplace_back(i + offset);
         const auto byte = static_cast<uint8_t>(hashed_message[i]);

diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_secp256r1.test.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ecdsa_secp256r1.test.cpp
@@ -23,7 +23,7 @@ size_t generate_r1_constraints(EcdsaSecp256r1Constraint& ecdsa_r1_constraint,
     std::vector<uint32_t> pub_x_indices_in;
     std::vector<uint32_t> pub_y_indices_in;
     std::vector<uint32_t> signature_in;
-    size_t offset = 1;
+    size_t offset = 0;
     for (size_t i = 0; i < hashed_message.size(); ++i) {
         message_in.emplace_back(i + offset);
         const auto byte = static_cast<uint8_t>(hashed_message[i]);