fix: challenge generation update

barretenberg/cpp/src/barretenberg/honk/transcript/transcript.hpp

@@ -71,32 +71,46 @@ template <typename FF> class BaseTranscript {
   private:
     static constexpr size_t MIN_BYTES_PER_CHALLENGE = 128 / 8; // 128 bit challenges
 
-    size_t round_number = 0;
+    size_t round_number = 0;        // current round for manifest
+    bool is_first_challenge = true; // indicates if this is the first challenge this transcript is generating
     std::array<uint8_t, HASH_OUTPUT_SIZE> previous_challenge_buffer{}; // default-initialized to zeros
     std::vector<uint8_t> current_round_data;
 
     // "Manifest" object that records a summary of the transcript interactions
     TranscriptManifest manifest;
 
     /**
-     * @brief Compute c_next = H( Compress(c_prev || round_buffer) )
-     *
+     * @brief Compute next challenge c_next = H( Compress(c_prev || round_buffer) )
+     * @details This function computes a new challenge for the current round using the previous challenge
+     * and the current round data, if they are exist. It clears the current_round_data if nonempty after
+     * computing the challenge to minimize how much we compress. It also sets previous_challenge_buffer
+     * to the current challenge buffer to set up next function call.
      * @return std::array<uint8_t, HASH_OUTPUT_SIZE>
      */
-    [[nodiscard]] std::array<uint8_t, HASH_OUTPUT_SIZE> get_next_challenge_buffer() const
+    [[nodiscard]] std::array<uint8_t, HASH_OUTPUT_SIZE> get_next_challenge_buffer()
     {
-        // Prevent challenge generation if nothing was sent by the prover.
-        ASSERT(!current_round_data.empty());
+        // Prevent challenge generation if this is the first challenge we're generating,
+        // AND nothing was sent by the prover.
+        if (is_first_challenge) {
+            ASSERT(!current_round_data.empty());
+        }
 
-        // concatenate the hash of the previous round (if not the first round) with the current round data.
+        // concatenate the previous challenge (if this is not the first challenge) with the current round data.
         // TODO(Adrian): Do we want to use a domain separator as the initial challenge buffer?
         // We could be cheeky and use the hash of the manifest as domain separator, which would prevent us from having
         // to domain separate all the data. (See https://safe-hash.dev)
         std::vector<uint8_t> full_buffer;
-        if (round_number > 0) {
+        if (!is_first_challenge) {
+            // if not the first challenge, we can use the previous_challenge_buffer
             full_buffer.insert(full_buffer.end(), previous_challenge_buffer.begin(), previous_challenge_buffer.end());
+        } else {
+            // Update is_first_challenge for the future
+            is_first_challenge = false;
+        }
+        if (!current_round_data.empty()) {
+            full_buffer.insert(full_buffer.end(), current_round_data.begin(), current_round_data.end());
+            current_round_data.clear(); // clear the round data buffer since it has been used
         }
-        full_buffer.insert(full_buffer.end(), current_round_data.begin(), current_round_data.end());
 
         // Pre-hash the full buffer to minimize the amount of data passed to the cryptographic hash function.
         // Only a collision-resistant hash-function like Pedersen is required for this step.
@@ -109,7 +123,8 @@ template <typename FF> class BaseTranscript {
 
         std::array<uint8_t, HASH_OUTPUT_SIZE> new_challenge_buffer;
         std::copy_n(base_hash.begin(), HASH_OUTPUT_SIZE, new_challenge_buffer.begin());
-
+        // update previous challenge buffer for next time we call this function
+        previous_challenge_buffer = new_challenge_buffer;
         return new_challenge_buffer;
     };
 
@@ -131,8 +146,11 @@ template <typename FF> class BaseTranscript {
   public:
     /**
      * @brief After all the prover messages have been sent, finalize the round by hashing all the data and then create
-     * the number of requested challenges which will be increasing powers of the first challenge.  Finally, reset the
-     * state in preparation for the next round.
+     * the number of requested challenges.
+     * @details Challenges are generated by iteratively hashing over the previous challenge, using
+     * get_next_challenge_buffer().
+     * TODO(#741): Optimizations for this function include generalizing type of hash, splitting hashes into
+     * multiple challenges.
      *
      * @param labels human-readable names for the challenges for the manifest
      * @return std::array<FF, num_challenges> challenges for this round.
@@ -145,26 +163,25 @@ template <typename FF> class BaseTranscript {
         manifest.add_challenge(round_number, labels...);
 
         // Compute the new challenge buffer from which we derive the challenges.
-        auto next_challenge_buffer = get_next_challenge_buffer();
 
         // Create challenges from bytes.
         std::array<FF, num_challenges> challenges{};
 
-        std::array<uint8_t, sizeof(FF)> field_element_buffer{};
-        std::copy_n(next_challenge_buffer.begin(), HASH_OUTPUT_SIZE, field_element_buffer.begin());
-
-        challenges[0] = from_buffer<FF>(field_element_buffer);
-
-        // TODO(#583): rework the transcript to have a better structure and be able to produce a variable amount of
-        // challenges that are not powers of each other
-        for (size_t i = 1; i < num_challenges; i++) {
-            challenges[i] = challenges[i - 1] * challenges[0];
+        // Generate the challenges by iteratively hashing over the previous challenge.
+        for (size_t i = 0; i < num_challenges; i++) {
+            auto next_challenge_buffer = get_next_challenge_buffer(); // get next challenge buffer
+            std::array<uint8_t, sizeof(FF)> field_element_buffer{};
+            // copy half of the hash to lower 128 bits of challenge
+            // Note: because of how read() from buffers to fields works (in field_declarations.hpp),
+            // we use the later half of the buffer
+            std::copy_n(next_challenge_buffer.begin(),
+                        HASH_OUTPUT_SIZE / 2,
+                        field_element_buffer.begin() + HASH_OUTPUT_SIZE / 2);
+            challenges[i] = from_buffer<FF>(field_element_buffer);
         }
 
         // Prepare for next round.
         ++round_number;
-        current_round_data.clear();
-        previous_challenge_buffer = next_challenge_buffer;
 
         return challenges;
     }

barretenberg/cpp/src/barretenberg/honk/transcript/transcript.test.cpp

@@ -139,8 +139,6 @@ TEST_F(UltraTranscriptTests, ProverManifestConsistency)
     auto manifest_expected = construct_ultra_honk_manifest(instance->proving_key->circuit_size);
     auto prover_manifest = prover.transcript.get_manifest();
     // Note: a manifest can be printed using manifest.print()
-    prover_manifest.print();
-    manifest_expected.print();
     for (size_t round = 0; round < manifest_expected.size(); ++round) {
         ASSERT_EQ(prover_manifest[round], manifest_expected[round]) << "Prover manifest discrepency in round " << round;
     }
@@ -171,9 +169,6 @@ TEST_F(UltraTranscriptTests, VerifierManifestConsistency)
     auto verifier = composer.create_verifier(instance);
     verifier.verify_proof(proof);
 
-    prover.transcript.print();
-    verifier.transcript.print();
-
     // Check consistency between the manifests generated by the prover and verifier
     auto prover_manifest = prover.transcript.get_manifest();
     auto verifier_manifest = verifier.transcript.get_manifest();
@@ -185,6 +180,30 @@ TEST_F(UltraTranscriptTests, VerifierManifestConsistency)
     }
 }
 
+/**
+ * @brief Check that multiple challenges can be generated and sanity check
+ * @details We generate 6 challenges that are each 128 bits, and check that they are not 0.
+ *
+ */
+TEST_F(UltraTranscriptTests, ChallengeGenerationTest)
+{
+    // initialized with random value sent to verifier
+    auto transcript = ProverTranscript<FF>::init_empty();
+    // test a bunch of challenges
+    auto challenges = transcript.get_challenges("a", "b", "c", "d", "e", "f");
+    // check they are not 0
+    for (size_t i = 0; i < challenges.size(); ++i) {
+        ASSERT_NE(challenges[i], 0) << "Challenge " << i << " is 0";
+    }
+    constexpr uint32_t random_val{ 17 }; // arbitrary
+    transcript.send_to_verifier("random val", random_val);
+    // test more challenges
+    auto [a, b, c] = transcript.get_challenges("a", "b", "c");
+    ASSERT_NE(a, 0) << "Challenge a is 0";
+    ASSERT_NE(b, 0) << "Challenge a is 0";
+    ASSERT_NE(b, 0) << "Challenge a is 0";
+}
+
 TEST_F(UltraTranscriptTests, FoldingManifestTest)
 {
     using Flavor = flavor::Ultra;
@@ -216,9 +235,6 @@ TEST_F(UltraTranscriptTests, FoldingManifestTest)
     auto prover_res = prover.fold_instances();
     verifier.fold_public_parameters(prover_res.folding_data);
 
-    prover.transcript.print();
-    verifier.transcript.print();
-
     // Check consistency between the manifests generated by the prover and verifier
     auto prover_manifest = prover.transcript.get_manifest();
     auto verifier_manifest = verifier.transcript.get_manifest();