chore: field_conversion clean-up/int audit

barretenberg/cpp/scripts/test_civc_standalone_vks_havent_changed.sh

@@ -13,7 +13,7 @@ cd ..
 # - Generate a hash for versioning: sha256sum bb-civc-inputs.tar.gz
 # - 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
 # Note: In case of the "Test suite failed to run ... Unexpected token 'with' " error, need to run: docker pull aztecprotocol/build:3.0
-pinned_short_hash="04f38201"
+pinned_short_hash="e70f08be"
 pinned_civc_inputs_url="https://aztec-ci-artifacts.s3.us-east-2.amazonaws.com/protocol/bb-civc-inputs-${pinned_short_hash}.tar.gz"
 
 function compress_and_upload {

barretenberg/cpp/src/barretenberg/dsl/acir_format/civc_recursion_constraints.cpp

@@ -111,7 +111,7 @@ create_civc_recursion_constraints(Builder& builder,
     }
 
     // Recursively verify CIVC proof
-    auto mega_vk = std::make_shared<VerificationKey>(builder, key_fields);
+    auto mega_vk = std::make_shared<VerificationKey>(key_fields);
     auto mega_vk_and_hash = std::make_shared<RecursiveVKAndHash>(mega_vk, vk_hash);
     ClientIVCRecursiveVerifier::StdlibProof stdlib_proof(proof_fields, input.public_inputs.size());
 

barretenberg/cpp/src/barretenberg/dsl/acir_format/honk_recursion_constraint.cpp

@@ -141,7 +141,7 @@ HonkRecursionConstraintOutput<typename Flavor::CircuitBuilder> create_honk_recur
     }
 
     // Recursively verify the proof
-    auto vkey = std::make_shared<RecursiveVerificationKey>(builder, key_fields);
+    auto vkey = std::make_shared<RecursiveVerificationKey>(key_fields);
     auto vk_and_hash = std::make_shared<RecursiveVKAndHash>(vkey, vk_hash);
     RecursiveVerifier verifier(&builder, vk_and_hash);
     UltraRecursiveVerifierOutput<Builder> verifier_output = verifier.template verify_proof<IO>(proof_fields);

barretenberg/cpp/src/barretenberg/flavor/mega_recursive_flavor.hpp

@@ -140,18 +140,18 @@ template <typename BuilderType> class MegaRecursiveFlavor_ {
          * @param builder
          * @param elements
          */
-        VerificationKey(CircuitBuilder& builder, std::span<FF> elements)
+        VerificationKey(std::span<FF> elements)
         {
             using namespace bb::stdlib::field_conversion;
 
             size_t num_frs_read = 0;
 
-            this->log_circuit_size = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->num_public_inputs = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->pub_inputs_offset = deserialize_from_frs<FF>(builder, elements, num_frs_read);
+            this->log_circuit_size = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->num_public_inputs = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->pub_inputs_offset = deserialize_from_frs<FF>(elements, num_frs_read);
 
             for (Commitment& commitment : this->get_all()) {
-                commitment = deserialize_from_frs<Commitment>(builder, elements, num_frs_read);
+                commitment = deserialize_from_frs<Commitment>(elements, num_frs_read);
             }
 
             if (num_frs_read != elements.size()) {
@@ -174,7 +174,7 @@ template <typename BuilderType> class MegaRecursiveFlavor_ {
             for (const auto& idx : witness_indices) {
                 vk_fields.emplace_back(FF::from_witness_index(&builder, idx));
             }
-            return VerificationKey(builder, vk_fields);
+            return VerificationKey(vk_fields);
         }
 
         /**

barretenberg/cpp/src/barretenberg/flavor/ultra_recursive_flavor.hpp

@@ -140,18 +140,18 @@ template <typename BuilderType> class UltraRecursiveFlavor_ {
          * @param builder
          * @param elements
          */
-        VerificationKey(CircuitBuilder& builder, std::span<FF> elements)
+        VerificationKey(std::span<FF> elements)
         {
             using namespace bb::stdlib::field_conversion;
 
             size_t num_frs_read = 0;
 
-            this->log_circuit_size = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->num_public_inputs = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->pub_inputs_offset = deserialize_from_frs<FF>(builder, elements, num_frs_read);
+            this->log_circuit_size = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->num_public_inputs = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->pub_inputs_offset = deserialize_from_frs<FF>(elements, num_frs_read);
 
             for (Commitment& commitment : this->get_all()) {
-                commitment = deserialize_from_frs<Commitment>(builder, elements, num_frs_read);
+                commitment = deserialize_from_frs<Commitment>(elements, num_frs_read);
             }
         }
 
@@ -170,7 +170,7 @@ template <typename BuilderType> class UltraRecursiveFlavor_ {
             for (const auto& idx : witness_indices) {
                 vk_fields.emplace_back(FF::from_witness_index(&builder, idx));
             }
-            return VerificationKey(builder, vk_fields);
+            return VerificationKey(vk_fields);
         }
     };
 

barretenberg/cpp/src/barretenberg/flavor/ultra_rollup_recursive_flavor.hpp

@@ -86,18 +86,18 @@ template <typename BuilderType> class UltraRollupRecursiveFlavor_ : public Ultra
          * @param builder
          * @param elements
          */
-        VerificationKey(CircuitBuilder& builder, std::span<FF> elements)
+        VerificationKey(std::span<FF> elements)
         {
             using namespace bb::stdlib::field_conversion;
 
             size_t num_frs_read = 0;
 
-            this->log_circuit_size = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->num_public_inputs = deserialize_from_frs<FF>(builder, elements, num_frs_read);
-            this->pub_inputs_offset = deserialize_from_frs<FF>(builder, elements, num_frs_read);
+            this->log_circuit_size = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->num_public_inputs = deserialize_from_frs<FF>(elements, num_frs_read);
+            this->pub_inputs_offset = deserialize_from_frs<FF>(elements, num_frs_read);
 
             for (Commitment& commitment : this->get_all()) {
-                commitment = deserialize_from_frs<Commitment>(builder, elements, num_frs_read);
+                commitment = deserialize_from_frs<Commitment>(elements, num_frs_read);
             }
         }
 
@@ -116,7 +116,7 @@ template <typename BuilderType> class UltraRollupRecursiveFlavor_ : public Ultra
             for (const auto& idx : witness_indices) {
                 vk_fields.emplace_back(FF::from_witness_index(&builder, idx));
             }
-            return VerificationKey(builder, vk_fields);
+            return VerificationKey(vk_fields);
         }
     };
 

barretenberg/cpp/src/barretenberg/stdlib/eccvm_verifier/eccvm_recursive_verifier.test.cpp

@@ -139,7 +139,7 @@ class ECCVMRecursiveTests : public ::testing::Test {
         }
 
         // Check that the size of the recursive verifier is consistent with historical expectation
-        uint32_t NUM_GATES_EXPECTED = 213923;
+        uint32_t NUM_GATES_EXPECTED = 213775;
         ASSERT_EQ(static_cast<uint32_t>(outer_circuit.get_num_finalized_gates()), NUM_GATES_EXPECTED)
             << "Ultra-arithmetized ECCVM Recursive verifier gate count changed! Update this value if you are sure this "
                "is expected.";

barretenberg/cpp/src/barretenberg/stdlib/goblin_verifier/goblin_recursive_verifier.test.cpp

@@ -25,16 +25,55 @@ class GoblinRecursiveVerifierTests : public testing::Test {
     using RecursiveCommitment = GoblinRecursiveVerifier::MergeVerifier::Commitment;
     using MergeCommitments = MergeVerifier::InputCommitments;
     using RecursiveMergeCommitments = GoblinRecursiveVerifier::MergeVerifier::InputCommitments;
-
+    using FF = TranslatorFlavor::FF;
+    using BF = TranslatorFlavor::BF;
     static void SetUpTestSuite() { bb::srs::init_file_crs_factory(bb::srs::bb_crs_path()); }
 
+    // Compute the size of a Translator commitment (in bb::fr's)
+    static constexpr size_t comm_frs = bb::field_conversion::calc_num_bn254_frs<Commitment>(); // 4
+    static constexpr size_t eval_frs = bb::field_conversion::calc_num_bn254_frs<FF>();         // 1
+
+    // The `op` wire commitment is currently the second element of the proof, following the
+    // `accumulated_result` which is a BN254 BaseField element.
+    static constexpr size_t offset = bb::field_conversion::calc_num_bn254_frs<BF>();
+
     struct ProverOutput {
         GoblinProof proof;
         Goblin::VerificationKey verifier_input;
         MergeCommitments merge_commitments;
         RecursiveMergeCommitments recursive_merge_commitments;
     };
+    // TODO(https://github.com/AztecProtocol/barretenberg/issues/1298):
+    // Better recursion testing - create more flexible proof tampering tests.
+    // Modify the `op` commitment which a part of the Merge protocol.
+    static void tamper_with_op_commitment(HonkProof& translator_proof)
+    {
+
+        // Extract `op` fields and convert them to a Commitment object
+        auto element_frs = std::span{ translator_proof }.subspan(offset, comm_frs);
+        auto op_commitment = NativeTranscriptParams::template deserialize<Commitment>(element_frs);
+        // Modify the commitment
+        op_commitment = op_commitment * FF(2);
+        // Serialize the tampered commitment into the proof (overwriting the valid one).
+        auto op_commitment_reserialized = bb::NativeTranscriptParams::serialize(op_commitment);
+        std::copy(op_commitment_reserialized.begin(),
+                  op_commitment_reserialized.end(),
+                  translator_proof.begin() + static_cast<std::ptrdiff_t>(offset));
+    };
+
+    // Translator proof ends with [..., Libra:quotient_eval, Shplonk:Q, KZG:W]. We invalidate the proof by multiplying
+    // the eval by 2 (it leads to a Libra consistency check failure).
+    static void tamper_with_libra_eval(HonkProof& translator_proof)
+    {
+        // Proof tail size
+        static constexpr size_t tail_size = 2 * comm_frs + eval_frs; // 2*4 + 1 = 9
+
+        // Index of the target field (one fr) from the beginning
+        const size_t idx = translator_proof.size() - tail_size;
 
+        // Tamper: multiply by 2 (or tweak however you like)
+        translator_proof[idx] = translator_proof[idx] + translator_proof[idx];
+    };
     /**
      * @brief Create a goblin proof and the VM verification keys needed by the goblin recursive verifier
      *
@@ -208,13 +247,7 @@ TEST_F(GoblinRecursiveVerifierTests, TranslatorFailure)
     // Tamper with the Translator proof preamble
     {
         GoblinProof tampered_proof = proof;
-        for (auto& val : tampered_proof.translator_proof) {
-            if (val > 0) { // tamper by finding the first non-zero value and incrementing it by 1
-                val += 1;
-                break;
-            }
-        }
-
+        tamper_with_op_commitment(tampered_proof.translator_proof);
         Builder builder;
 
         RecursiveMergeCommitments recursive_merge_commitments;
@@ -232,18 +265,10 @@ TEST_F(GoblinRecursiveVerifierTests, TranslatorFailure)
             verifier.verify(tampered_proof, recursive_merge_commitments, MergeSettings::APPEND);
         EXPECT_FALSE(CircuitChecker::check(builder));
     }
-    // Tamper with the Translator proof non-preamble values
+    // Tamper with the Translator proof non - preamble values
     {
         auto tampered_proof = proof;
-        int seek = 10;
-        for (auto& val : tampered_proof.translator_proof) {
-            if (val > 0) { // tamper by finding the tenth non-zero value and incrementing it by 1
-                if (--seek == 0) {
-                    val += 1;
-                    break;
-                }
-            }
-        }
+        tamper_with_libra_eval(tampered_proof.translator_proof);
 
         Builder builder;
 
@@ -296,9 +321,6 @@ TEST_F(GoblinRecursiveVerifierTests, TranslatorMergeConsistencyFailure)
 {
 
     {
-        using Commitment = TranslatorFlavor::Commitment;
-        using FF = TranslatorFlavor::FF;
-        using BF = TranslatorFlavor::BF;
 
         Builder builder;
 
@@ -310,27 +332,6 @@ TEST_F(GoblinRecursiveVerifierTests, TranslatorMergeConsistencyFailure)
         // Check natively that the proof is correct.
         EXPECT_TRUE(Goblin::verify(proof, merge_commitments, verifier_transcript, MergeSettings::APPEND));
 
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/1298):
-        // Better recursion testing - create more flexible proof tampering tests.
-        // Modify the `op` commitment which a part of the Merge protocol.
-        auto tamper_with_op_commitment = [](HonkProof& translator_proof) {
-            // Compute the size of a Translator commitment (in bb::fr's)
-            static constexpr size_t num_frs_comm = bb::field_conversion::calc_num_bn254_frs<Commitment>();
-            // The `op` wire commitment is currently the second element of the proof, following the
-            // `accumulated_result` which is a BN254 BaseField element.
-            static constexpr size_t offset = bb::field_conversion::calc_num_bn254_frs<BF>();
-            // Extract `op` fields and convert them to a Commitment object
-            auto element_frs = std::span{ translator_proof }.subspan(offset, num_frs_comm);
-            auto op_commitment = NativeTranscriptParams::template deserialize<Commitment>(element_frs);
-            // Modify the commitment
-            op_commitment = op_commitment * FF(2);
-            // Serialize the tampered commitment into the proof (overwriting the valid one).
-            auto op_commitment_reserialized = bb::NativeTranscriptParams::serialize(op_commitment);
-            std::copy(op_commitment_reserialized.begin(),
-                      op_commitment_reserialized.end(),
-                      translator_proof.begin() + static_cast<std::ptrdiff_t>(offset));
-        };
-
         tamper_with_op_commitment(proof.translator_proof);
         // Construct and check the Goblin Recursive Verifier circuit
 

barretenberg/cpp/src/barretenberg/stdlib/honk_verifier/ultra_recursive_verifier.test.cpp

@@ -293,7 +293,7 @@ template <typename RecursiveFlavor> class RecursiveVerifierTest : public testing
         }
         // Check the size of the recursive verifier
         if constexpr (std::same_as<RecursiveFlavor, MegaZKRecursiveFlavor_<UltraCircuitBuilder>>) {
-            uint32_t NUM_GATES_EXPECTED = 796978;
+            uint32_t NUM_GATES_EXPECTED = 801953;
             ASSERT_EQ(static_cast<uint32_t>(outer_circuit.get_num_finalized_gates()), NUM_GATES_EXPECTED)
                 << "MegaZKHonk Recursive verifier changed in Ultra gate count! Update this value if you "
                    "are sure this is expected.";

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup.hpp

@@ -45,6 +45,7 @@ template <class Builder_, class Fq, class Fr, class NativeGroup> class element {
     element();
     element(const typename NativeGroup::affine_element& input);
     element(const Fq& x, const Fq& y);
+    element(const Fq& x, const Fq& y, const bool_ct& is_infinity);
 
     element(const element& other);
     element(element&& other) noexcept;

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_goblin.hpp

@@ -56,6 +56,11 @@ template <class Builder_, class Fq, class Fr, class NativeGroup> class goblin_el
         , y(y)
         , _is_infinity(false)
     {}
+    goblin_element(const Fq& x, const Fq& y, const bool_ct is_infinity)
+        : x(x)
+        , y(y)
+        , _is_infinity(is_infinity)
+    {}
     goblin_element(const goblin_element& other) = default;
     goblin_element(goblin_element&& other) noexcept = default;
     goblin_element& operator=(const goblin_element& other) = default;

barretenberg/cpp/src/barretenberg/stdlib/primitives/biggroup/biggroup_impl.hpp

@@ -36,6 +36,13 @@ element<C, Fq, Fr, G>::element(const Fq& x_in, const Fq& y_in)
     , _is_infinity(x.get_context() ? x.get_context() : y.get_context(), false)
 {}
 
+template <typename C, class Fq, class Fr, class G>
+element<C, Fq, Fr, G>::element(const Fq& x_in, const Fq& y_in, const bool_ct& is_infinity)
+    : x(x_in)
+    , y(y_in)
+    , _is_infinity(is_infinity)
+{}
+
 template <typename C, class Fq, class Fr, class G>
 element<C, Fq, Fr, G>::element(const element& other)
     : x(other.x)

barretenberg/cpp/src/barretenberg/stdlib/primitives/field/field_conversion.cpp

@@ -5,46 +5,43 @@
 // =====================
 
 #include "barretenberg/stdlib/primitives/field/field_conversion.hpp"
-#include "barretenberg/common/assert.hpp"
-
 namespace bb::stdlib::field_conversion {
 
 /**
- * @brief Converts a challenge to a fq<Builder>
- * @details We sometimes need challenges that are a bb::fq element, so we need to convert the bb::fr challenge to a
- * bb::fq type. We do this by in a similar fashion to the convert_from_bn254_frs function that converts to a
- * fq<Builder>. In fact, we do call that function that the end, but we first have to split the fr<Builder> into two
- * pieces, one that is the 136 lower bits and one that is the 118 higher bits. Then, we can split these two pieces into
- * their bigfield limbs through convert_from_bn254_frs, which is actually just a bigfield constructor that takes in two
- * two-limb frs.
+ * @brief Converts an in-circuit `fr`element to an `fq`, i.e. `field_t` --> `bigfield`.
  *
- * TODO(https://github.com/AztecProtocol/barretenberg/issues/850): audit this function more carefully
+ * @details Our circuit builders are `fr`-native, which results in challenges being `field_t` elements. However,
+ * ECCVMRecursiveVerifier and IPA Recursive Verification need challenges that are `bigfield` elements. We do this in
+ * a similar fashion to the `convert_from_bn254_frs` function that converts to a `bigfield`. We split the `field_t`
+ * into two pieces, one that is the 136 lower bits and one that is the 118 higher bits, assert the correctness of the
+ * decomposition, and invoke the `bigfield` constructor.
  * @tparam Builder
  */
-template <typename Builder> fq<Builder> convert_to_grumpkin_fr(Builder& builder, const fr<Builder>& f)
+template <typename Builder> fq<Builder> convert_to_grumpkin_fr(Builder& builder, const fr<Builder>& fr_element)
 {
-    constexpr uint64_t NUM_BITS_IN_TWO_LIMBS = 2 * NUM_LIMB_BITS;                // 136
-    constexpr uint64_t UPPER_TWO_LIMB_BITS = TOTAL_BITS - NUM_BITS_IN_TWO_LIMBS; // 118
+    ASSERT(!fr_element.is_constant());
+    static constexpr uint64_t NUM_LIMB_BITS = fq<Builder>::NUM_LIMB_BITS;
+
+    constexpr uint64_t NUM_BITS_IN_TWO_LIMBS = 2 * NUM_LIMB_BITS; // 136
+
     constexpr uint256_t shift = (uint256_t(1) << NUM_BITS_IN_TWO_LIMBS);
     // split f into low_bits_in and high_bits_in
     constexpr uint256_t LIMB_MASK = shift - 1; // mask for upper 128 bits
-    const uint256_t value = f.get_value();
+    const uint256_t value = fr_element.get_value();
     const uint256_t low_val = static_cast<uint256_t>(value & LIMB_MASK);
     const uint256_t hi_val = static_cast<uint256_t>(value >> NUM_BITS_IN_TWO_LIMBS);
 
     fr<Builder> low{ witness_t<Builder>(&builder, low_val) };
     fr<Builder> hi{ witness_t<Builder>(&builder, hi_val) };
-    // range constrain low to 136 bits and hi to 118 bits
-    builder.create_range_constraint(low.witness_index, NUM_BITS_IN_TWO_LIMBS, "create_range_constraint");
-    builder.create_range_constraint(hi.witness_index, UPPER_TWO_LIMB_BITS, "create_range_constraint");
 
-    BB_ASSERT_EQ(static_cast<uint256_t>(low_val) + (static_cast<uint256_t>(hi_val) << NUM_BITS_IN_TWO_LIMBS), value);
-    // checks this decomposition low + hi * 2^64 = value with an assert_equal
-    auto sum = low + hi * shift;
-    builder.assert_equal(f.witness_index, sum.witness_index, "assert_equal");
+    BB_ASSERT_EQ(static_cast<uint256_t>(low_val) + (static_cast<uint256_t>(hi_val) << NUM_BITS_IN_TWO_LIMBS),
+                 value,
+                 "field_conversion: limb decomposition");
+    // check the decomposition low + hi * 2^136 = value in circuit
+    const fr<Builder> zero = fr<Builder>::from_witness_index(&builder, builder.zero_idx);
+    fr<Builder>::evaluate_linear_identity(hi * shift, low, -fr_element, zero);
 
-    std::vector<fr<Builder>> fr_vec{ low, hi };
-    return convert_from_bn254_frs<Builder, fq<Builder>>(builder, fr_vec);
+    return fq<Builder>(low, hi);
 }
 
 template fq<UltraCircuitBuilder> convert_to_grumpkin_fr<UltraCircuitBuilder>(UltraCircuitBuilder& builder,