test updates, comments and issue added

barretenberg/cpp/src/barretenberg/honk/composer/goblin/full_goblin_composer.test.cpp

@@ -101,13 +101,56 @@ class FullGoblinComposerTests : public ::testing::Test {
         // Store the commitment data for use by the prover of the next circuit
         op_queue->set_commitment_data(op_queue_commitments);
     }
+
+    /**
+     * @brief Construct and a verify a Honk proof
+     *
+     */
+    bool construct_and_verify_honk_proof(auto& composer, auto& builder)
+    {
+        auto instance = composer.create_instance(builder);
+        auto prover = composer.create_prover(instance);
+        auto verifier = composer.create_verifier(instance);
+        auto proof = prover.construct_proof();
+        bool verified = verifier.verify_proof(proof);
+
+        return verified;
+    }
+
+    /**
+     * @brief Construct and verify a Goblin ECC op queue merge proof
+     *
+     */
+    bool construct_and_verify_merge_proof(auto& composer, auto& op_queue)
+    {
+        auto merge_prover = composer.create_merge_prover(op_queue);
+        auto merge_verifier = composer.create_merge_verifier(10);
+        auto merge_proof = merge_prover.construct_proof();
+        bool verified = merge_verifier.verify_proof(merge_proof);
+
+        return verified;
+    }
+
+    /**
+     * @brief Construct and verify a Goblin ECC op queue merge proof
+     *
+     */
+    bool construct_and_verify_eccvm_proof(auto& composer, auto& builder)
+    {
+        auto prover = composer.create_prover(builder);
+        auto proof = prover.construct_proof();
+        auto verifier = composer.create_verifier(builder);
+        bool verified = verifier.verify_proof(proof);
+
+        return verified;
+    }
 };
 
 /**
  * @brief Test proof construction/verification for a circuit with ECC op gates, public inputs, and basic arithmetic
  * gates
  * @note We simulate op queue interactions with a previous circuit so the actual circuit under test utilizes an op queue
- * with non-empty 'previous' data. This avoid complications with zero-commitments etc.
+ * with non-empty 'previous' data. This avoids complications with zero-commitments etc.
  *
  */
 TEST_F(FullGoblinComposerTests, SimpleCircuit)
@@ -124,29 +167,27 @@ TEST_F(FullGoblinComposerTests, SimpleCircuit)
 
         generate_test_circuit(builder);
 
+        // The same composer is used to manage Honk and Merge prover/verifier
         auto composer = GoblinUltraComposer();
-        auto instance = composer.create_instance(builder);
-        auto prover = composer.create_prover(instance);
-        auto verifier = composer.create_verifier(instance);
-        auto proof = prover.construct_proof();
-        bool verified = verifier.verify_proof(proof);
-        EXPECT_EQ(verified, true);
+
+        // Construct and verify Ultra Goblin Honk proof
+        auto honk_verified = construct_and_verify_honk_proof(composer, builder);
+        EXPECT_TRUE(honk_verified);
+
+        // Construct and verify op queue merge proof
+        auto merge_verified = construct_and_verify_merge_proof(composer, op_queue);
+        EXPECT_TRUE(merge_verified);
     }
 
     // Construct an ECCVM circuit then generate and verify its proof
     {
         // Instantiate an ECCVM builder with the vm ops stored in the op queue
         auto builder = ECCVMBuilder(op_queue->raw_ops);
 
-        // // Can fiddle with one of the operands to trigger a failure
-        // builder.vm_operations[0].z1 *= 2;
-
+        // Construct and verify ECCVM proof
         auto composer = ECCVMComposer();
-        auto prover = composer.create_prover(builder);
-        auto proof = prover.construct_proof();
-        auto verifier = composer.create_verifier(builder);
-        bool verified = verifier.verify_proof(proof);
-        ASSERT_TRUE(verified);
+        auto eccvm_verified = construct_and_verify_eccvm_proof(composer, builder);
+        EXPECT_TRUE(eccvm_verified);
     }
 }
 
@@ -168,13 +209,16 @@ TEST_F(FullGoblinComposerTests, SimpleCircuitFailureCase)
 
         generate_test_circuit(builder);
 
+        // The same composer is used to manage Honk and Merge prover/verifier
         auto composer = GoblinUltraComposer();
-        auto instance = composer.create_instance(builder);
-        auto prover = composer.create_prover(instance);
-        auto verifier = composer.create_verifier(instance);
-        auto proof = prover.construct_proof();
-        bool verified = verifier.verify_proof(proof);
-        EXPECT_EQ(verified, true);
+
+        // Construct and verify Ultra Goblin Honk proof
+        auto honk_verified = construct_and_verify_honk_proof(composer, builder);
+        EXPECT_TRUE(honk_verified);
+
+        // Construct and verify op queue merge proof
+        auto merge_verified = construct_and_verify_merge_proof(composer, op_queue);
+        EXPECT_TRUE(merge_verified);
     }
 
     // Construct an ECCVM circuit then generate and verify its proof
@@ -185,12 +229,10 @@ TEST_F(FullGoblinComposerTests, SimpleCircuitFailureCase)
         // Fiddle with one of the operands to trigger a failure
         builder.vm_operations[0].z1 += 1;
 
+        // Construct and verify ECCVM proof
         auto composer = ECCVMComposer();
-        auto prover = composer.create_prover(builder);
-        auto proof = prover.construct_proof();
-        auto verifier = composer.create_verifier(builder);
-        bool verified = verifier.verify_proof(proof);
-        EXPECT_EQ(verified, false);
+        auto eccvm_verified = construct_and_verify_eccvm_proof(composer, builder);
+        EXPECT_FALSE(eccvm_verified);
     }
 }
 

barretenberg/cpp/src/barretenberg/honk/composer/ultra_composer.hpp

@@ -74,6 +74,12 @@ template <UltraFlavor Flavor> class UltraComposer_ {
     UltraProver_<Flavor> create_prover(std::shared_ptr<Instance>);
     UltraVerifier_<Flavor> create_verifier(std::shared_ptr<Instance>);
 
+    /**
+     * @brief Create Prover for Goblin ECC op queue merge protocol
+     *
+     * @param op_queue
+     * @return MergeProver_<Flavor>
+     */
     MergeProver_<Flavor> create_merge_prover(std::shared_ptr<ECCOpQueue> op_queue)
     {
         // Store the previous aggregate op queue size and update the current one
@@ -84,6 +90,12 @@ template <UltraFlavor Flavor> class UltraComposer_ {
         return MergeProver_<Flavor>(commitment_key, op_queue);
     }
 
+    /**
+     * @brief Create Verifier for Goblin ECC op queue merge protocol
+     *
+     * @param size Size of commitment key required to commit to shifted op queue contribution t_i
+     * @return MergeVerifier_<Flavor>
+     */
     MergeVerifier_<Flavor> create_merge_verifier(size_t size)
     {
         auto pcs_verification_key = std::make_unique<VerifierCommitmentKey>(size, crs_factory_);

barretenberg/cpp/src/barretenberg/honk/proof_system/goblin_merge/merge_prover.cpp

@@ -20,6 +20,9 @@ MergeProver_<Flavor>::MergeProver_(std::shared_ptr<CommitmentKey> commitment_key
  * M_{i-1}), where the shift magnitude M_{i-1} is the length of T_{i-1}. This protocol demonstrates that the aggregate
  * op queue has been constructed correctly via a simple Schwartz-Zippel check. Evaluations are proven via batched KZG.
  *
+ * TODO(#746): Prove connection between t_i^{shift}, committed to herein, and t_i, used in the main protocol. See issue
+ * for details (https://github.com/AztecProtocol/barretenberg/issues/746).
+ *
  * @tparam Flavor
  * @return plonk::proof&
  */