Address Luke's Comments on aztec3 -> master (#263)

* Add must_imply tests.

* use std honk composer.

* Add fail test.

* Added a test for `field_t::copy_as_new_witness`

* minor change.

* add test for `conditional_assign`

* Get rid of magic `15`

* Comment in pedersen lookup test.

* Remove unused/unnecessary condition.

* Fix postfix op in ecc field and add tests.

* Added `infinity` test.

* Fix fr test.

* Add `add_affine_test`.

cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_lookup.test.cpp

@@ -16,7 +16,29 @@ auto compute_expected(const grumpkin::fq exponent, size_t generator_offset)
     const auto lambda = grumpkin::fr::cube_root_of_unity();
     const auto mask = crypto::pedersen_hash::lookup::PEDERSEN_TABLE_SIZE - 1;
 
-    for (size_t i = 0; i < 15; ++i) {
+    /**
+     * Given an input scalar x, we split it into 9-bit slices:
+     * x = ( x_28 || x_27 || ... || x_2 || x_1 || x_0 )
+     *
+     * Note that the last slice x_28 is a 2-bit slice. Total = 2 + 9 * 28 = 254 bits.
+     *
+     * Algorithm:
+     *     hash = O;
+     *     hash += x_0 * G_0  +  x_1 * λ * G_0;
+     *     hash += x_2 * G_1  +  x_2 * λ * G_1;
+     *     ...
+     *     ...
+     *     hash += x_26 * G_13  +  x_27 * λ * G_13;
+     *     hash += x_27 * G_14;
+     *
+     * Our lookup tables stores the following:
+     *     1 -> (G_0, (λ * G_0))
+     *     2 -> (2G_0, 2(λ * G_0))
+     *     3 -> (3G_0, 3(λ * G_0))
+     *     ...
+     *   512 -> (512G_0, 512(λ * G_0))
+     */
+    for (size_t i = 0; i < (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2); ++i) {
         const auto slice_a = static_cast<size_t>(bits.data[0] & mask) + 1;
         bits >>= crypto::pedersen_hash::lookup::BITS_PER_TABLE;
         const auto slice_b = static_cast<size_t>(bits.data[0] & mask) + 1;
@@ -81,7 +103,7 @@ TEST(pedersen_lookup, hash_single)
 
     std::array<element, 2> accumulators;
 
-    for (size_t i = 0; i < 15; ++i) {
+    for (size_t i = 0; i < (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2); ++i) {
         const auto slice_a = static_cast<size_t>(bits.data[0] & mask) + 1;
         bits >>= crypto::pedersen_hash::lookup::BITS_PER_TABLE;
         const auto slice_b = static_cast<size_t>(bits.data[0] & mask) + 1;
@@ -115,7 +137,8 @@ TEST(pedersen_lookup, hash_pair)
 
     const fq result(crypto::pedersen_hash::lookup::hash_pair(left, right));
 
-    const affine_element expected(compute_expected(left, 0) + compute_expected(right, 15));
+    const affine_element expected(compute_expected(left, 0) +
+                                  compute_expected(right, (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)));
 
     EXPECT_EQ(result, expected.x);
 }
@@ -136,11 +159,16 @@ TEST(pedersen_lookup, merkle_damgard_compress)
 
     fq intermediate = (grumpkin::g1::affine_one * fr(iv + 1)).x;
     for (size_t i = 0; i < m; i++) {
-        intermediate = affine_element(compute_expected(intermediate, 0) + compute_expected(inputs[i], 15)).x;
+        intermediate =
+            affine_element(compute_expected(intermediate, 0) +
+                           compute_expected(inputs[i], (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                .x;
     }
 
     EXPECT_EQ(affine_element(result).x,
-              affine_element(compute_expected(intermediate, 0) + compute_expected(fq(m), 15)).x);
+              affine_element(compute_expected(intermediate, 0) +
+                             compute_expected(fq(m), (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                  .x);
 }
 
 TEST(pedersen_lookup, merkle_damgard_compress_multiple_iv)
@@ -164,14 +192,22 @@ TEST(pedersen_lookup, merkle_damgard_compress_multiple_iv)
     for (size_t i = 0; i < 2 * m; i++) {
         if ((i & 1) == 0) {
             const auto iv = (grumpkin::g1::affine_one * fr(ivs[i >> 1] + 1)).x;
-            intermediate = affine_element(compute_expected(intermediate, 0) + compute_expected(iv, 15)).x;
+            intermediate =
+                affine_element(compute_expected(intermediate, 0) +
+                               compute_expected(iv, (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                    .x;
         } else {
-            intermediate = affine_element(compute_expected(intermediate, 0) + compute_expected(inputs[i >> 1], 15)).x;
+            intermediate = affine_element(compute_expected(intermediate, 0) +
+                                          compute_expected(inputs[i >> 1],
+                                                           (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                               .x;
         }
     }
 
     EXPECT_EQ(affine_element(result).x,
-              affine_element(compute_expected(intermediate, 0) + compute_expected(fq(m), 15)).x);
+              affine_element(compute_expected(intermediate, 0) +
+                             compute_expected(fq(m), (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                  .x);
 }
 
 TEST(pedersen_lookup, merkle_damgard_tree_compress)
@@ -193,16 +229,25 @@ TEST(pedersen_lookup, merkle_damgard_tree_compress)
     std::vector<fq> temp;
     for (size_t i = 0; i < m; i++) {
         const fq iv_term = (grumpkin::g1::affine_one * fr(ivs[i] + 1)).x;
-        temp.push_back(affine_element(compute_expected(iv_term, 0) + compute_expected(inputs[i], 15)).x);
+        temp.push_back(
+            affine_element(compute_expected(iv_term, 0) +
+                           compute_expected(inputs[i], (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                .x);
     }
 
     const size_t logm = numeric::get_msb(m);
     for (size_t j = 1; j <= logm; j++) {
         const size_t nodes = (1UL << (logm - j));
         for (size_t i = 0; i < nodes; i++) {
-            temp[i] = affine_element(compute_expected(temp[2 * i], 0) + compute_expected(temp[2 * i + 1], 15)).x;
+            temp[i] = affine_element(
+                          compute_expected(temp[2 * i], 0) +
+                          compute_expected(temp[2 * i + 1], (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                          .x;
         }
     }
 
-    EXPECT_EQ(affine_element(result).x, affine_element(compute_expected(temp[0], 0) + compute_expected(fq(m), 15)).x);
+    EXPECT_EQ(affine_element(result).x,
+              affine_element(compute_expected(temp[0], 0) +
+                             compute_expected(fq(m), (crypto::pedersen_hash::lookup::NUM_PEDERSEN_TABLES / 2)))
+                  .x);
 }

cpp/src/barretenberg/ecc/curves/bn254/fr.test.cpp

@@ -86,6 +86,35 @@ TEST(fr, sub)
     EXPECT_EQ((result == expected), true);
 }
 
+TEST(fr, plus_equals)
+{
+    fr a{ 0x5def, 0x00, 0x00, 0x00 };
+    fr a_copy = a;
+    a += 2;
+    fr expected = a_copy + 2;
+    EXPECT_EQ((a == expected), true);
+
+    a += 3;
+    expected = a_copy + 5;
+    EXPECT_EQ((a == expected), true);
+}
+
+TEST(fr, prefix_increment)
+{
+    fr a{ 0x5def, 0x00, 0x00, 0x00 };
+    fr b = ++a;
+    EXPECT_EQ(b, a);
+}
+
+TEST(fr, postfix_increment)
+{
+    fr a{ 0x5def, 0x00, 0x00, 0x00 };
+    fr a_old = a;
+    fr b = a++;
+    EXPECT_EQ(b, a_old);
+    EXPECT_EQ(a, a_old + 1);
+}
+
 TEST(fr, to_montgomery_form)
 {
     fr result{ 0x01, 0x00, 0x00, 0x00 };

cpp/src/barretenberg/ecc/curves/bn254/g1.test.cpp

@@ -145,6 +145,15 @@ TEST(g1, add_exception_test_infinity)
     EXPECT_EQ(rhs == result, true);
 }
 
+TEST(g1, test_infinity)
+{
+    g1::affine_element inf_affine = g1::affine_element::infinity();
+    EXPECT_EQ(inf_affine.is_point_at_infinity(), true);
+
+    g1::element inf_element = g1::element::infinity();
+    EXPECT_EQ(inf_element.is_point_at_infinity(), true);
+}
+
 TEST(g1, add_exception_test_dbl)
 {
     g1::element lhs = g1::element::random_element();
@@ -160,6 +169,19 @@ TEST(g1, add_exception_test_dbl)
     EXPECT_EQ(result == expected, true);
 }
 
+TEST(g1, add_affine_test)
+{
+    g1::element lhs = g1::element::random_element();
+    g1::affine_element lhs_affine = g1::affine_element(lhs);
+
+    g1::element rhs = g1::element::random_element();
+    g1::affine_element rhs_affine = g1::affine_element(rhs);
+
+    g1::element expected = lhs + rhs;
+    g1::affine_element result = lhs_affine + rhs_affine;
+    EXPECT_EQ(g1::element(result) == expected, true);
+}
+
 TEST(g1, add_dbl_consistency)
 {
     g1::element a = g1::element::random_element();

cpp/src/barretenberg/ecc/fields/field_impl.hpp

@@ -136,8 +136,9 @@ template <class T> constexpr field<T> field<T>::operator++() noexcept
 
 template <class T> constexpr field<T> field<T>::operator++(int) noexcept
 {
-    // TODO check if this is correct.
-    return *this += 1;
+    field<T> value_before_incrementing = *this;
+    *this += 1;
+    return value_before_incrementing;
 }
 
 /**

cpp/src/barretenberg/ecc/groups/element_impl.hpp

@@ -489,9 +489,6 @@ template <class Fq, class Fr, class T> element<Fq, Fr, T> element<Fq, Fr, T>::in
 {
     element<Fq, Fr, T> e;
     e.self_set_infinity();
-    if (!e.is_point_at_infinity()) {
-        info("yup, it's infinity");
-    };
     return e;
 }
 

cpp/src/barretenberg/stdlib/primitives/bool/bool.cpp

@@ -461,25 +461,54 @@ void bool_t<ComposerContext>::must_imply(const bool_t& other, std::string const&
 template <typename ComposerContext>
 void bool_t<ComposerContext>::must_imply(const std::vector<std::pair<bool_t, std::string>>& conds) const
 {
-    bool_t acc = true; // will accumulate the conjunctions of each condition (i.e. `&&` of each)
+    // Extract the composer
+    const bool_t this_bool = *this;
+    ComposerContext* ctx = this_bool.get_context();
+    bool composer_found = (ctx != nullptr);
+    for (size_t i = 0; i < conds.size(); i++) {
+        if (!composer_found) {
+            ctx = conds[i].first.get_context();
+            composer_found = (ctx != nullptr);
+        }
+    }
+
+    // If no composer is found, all of the bool_t's must be constants.
+    // In that case, we enforce a static assertion to check must_imply condition holds.
+    // If all of your inputs do this function are constants and they don't obey a condition,
+    // this function will panic at those static assertions.
+    if (!composer_found) {
+        bool is_const = this_bool.is_constant();
+        bool result = !this_bool.get_value();
+        bool acc = true;
+        for (size_t i = 0; i < conds.size(); i++) {
+            is_const &= conds[i].first.is_constant();
+            acc &= conds[i].first.get_value();
+        }
+        result |= acc;
+        ASSERT(is_const == true);
+        ASSERT(result == true);
+    }
+
+    bool_t acc = witness_t(ctx, true); // will accumulate the conjunctions of each condition (i.e. `&&` of each)
     const bool this_val = this->get_value();
+    bool error_found = false;
+    std::string error_msg;
 
     for (size_t i = 0; i < conds.size(); ++i) {
         const bool_t& cond = conds[i].first;
         const std::string& msg = conds[i].second;
         const bool cond_val = cond.get_value();
 
         // If this does NOT imply that, throw the error msg of that condition.
-        if (!(!this_val || cond_val)) {
-            // TODO: make it so the first failure message given to the composer persists, and cannot be overwritten by
-            // subsequent failure msgs.
-            context->failure(msg);
+        if (!(!this_val || cond_val) && !error_found) {
+            error_found = true;
+            error_msg = msg;
         }
 
         acc &= cond;
     }
 
-    (this->implies(acc)).assert_equal(true, "multi implication fail");
+    (this->implies(acc)).assert_equal(true, format("multi implication fail: ", error_msg));
 }
 
 // A "double-implication" (<=>),