feat: read_calldata (#5409)

Add a `read_calldata` method to clarify and robustify the interface to
calldata. Update the expository DataBus test accordingly.

Closes #821 (I was
proposing something weird in that issue but all I really wanted was for
the updating of read counts to be automated. That's achieved through
simply placing it in read_calldata).

cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.cpp

@@ -35,11 +35,7 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::add_gates_to_ensure_
     add_public_calldata(FF(25)); // ensure there is at least one entry in calldata
     uint32_t raw_read_idx = 0;   // read first entry in calldata
     auto read_idx = this->add_variable(raw_read_idx);
-    FF calldata_value = this->get_variable(public_calldata[raw_read_idx]);
-    auto value_idx = this->add_variable(calldata_value);
-    create_calldata_lookup_gate({ read_idx, value_idx });
-    // TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
-    calldata_read_counts[raw_read_idx]++;
+    read_calldata(read_idx);
 
     // mock a poseidon external gate, with all zeros as input
     this->blocks.poseidon_external.populate_wires(this->zero_idx, this->zero_idx, this->zero_idx, this->zero_idx);
@@ -220,14 +216,41 @@ template <typename FF> void GoblinUltraCircuitBuilder_<FF>::set_goblin_ecc_op_co
     equality_op_idx = this->put_constant_variable(FF(EccOpCode::EQUALITY));
 }
 
+/**
+ * @brief Read from calldata
+ * @details Creates a calldata lookup gate based on the read data
+ *
+ * @tparam FF
+ * @param read_idx_witness_idx Variable index of the read index
+ * @return uint32_t Variable index of the result of the read
+ */
+template <typename FF> uint32_t GoblinUltraCircuitBuilder_<FF>::read_calldata(const uint32_t& read_idx_witness_idx)
+{
+    // Get the raw index into the calldata
+    const uint32_t read_idx = static_cast<uint32_t>(uint256_t(this->get_variable(read_idx_witness_idx)));
+
+    // Ensure that the read index is valid
+    ASSERT(read_idx < public_calldata.size());
+
+    // Create a variable corresponding to the result of the read. Note that we do not in general connect reads from
+    // calldata via copy constraints (i.e. we create a unique variable for the result of each read)
+    FF calldata_value = this->get_variable(public_calldata[read_idx]);
+    uint32_t value_witness_idx = this->add_variable(calldata_value);
+
+    create_calldata_read_gate({ read_idx_witness_idx, value_witness_idx });
+    calldata_read_counts[read_idx]++;
+
+    return value_witness_idx;
+}
+
 /**
  * @brief Create a calldata lookup/read gate
  *
  * @tparam FF
  * @param databus_lookup_gate_ witness indices corresponding to: calldata index, calldata value
  */
 template <typename FF>
-void GoblinUltraCircuitBuilder_<FF>::create_calldata_lookup_gate(const databus_lookup_gate_<FF>& in)
+void GoblinUltraCircuitBuilder_<FF>::create_calldata_read_gate(const databus_lookup_gate_<FF>& in)
 {
     auto& block = this->blocks.busread;
     block.populate_wires(in.value, in.index, this->zero_idx, this->zero_idx);

cpp/src/barretenberg/proof_system/circuit_builder/goblin_ultra_circuit_builder.hpp

@@ -42,6 +42,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
     void populate_ecc_op_wires(const ecc_op_tuple& in);
     ecc_op_tuple decompose_ecc_operands(uint32_t op, const g1::affine_element& point, const FF& scalar = FF::zero());
     void set_goblin_ecc_op_code_constant_variables();
+    void create_calldata_read_gate(const databus_lookup_gate_<FF>& in);
 
   public:
     GoblinUltraCircuitBuilder_(const size_t size_hint = 0,
@@ -136,7 +137,7 @@ template <typename FF> class GoblinUltraCircuitBuilder_ : public UltraCircuitBui
         return index;
     }
 
-    void create_calldata_lookup_gate(const databus_lookup_gate_<FF>& in);
+    uint32_t read_calldata(const uint32_t& read_idx_witness_idx);
 
     void create_poseidon2_external_gate(const poseidon2_external_gate_<FF>& in);
     void create_poseidon2_internal_gate(const poseidon2_internal_gate_<FF>& in);

cpp/src/barretenberg/sumcheck/instance/prover_instance.cpp

@@ -48,7 +48,6 @@ void ProverInstance_<Flavor>::construct_databus_polynomials(Circuit& circuit)
     // Note: We do not utilize a zero row for databus columns
     for (size_t idx = 0; idx < circuit.public_calldata.size(); ++idx) {
         public_calldata[idx] = circuit.get_variable(circuit.public_calldata[idx]);
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
         calldata_read_counts[idx] = circuit.calldata_read_counts[idx];
     }
 

cpp/src/barretenberg/ultra_honk/databus_composer.test.cpp

@@ -23,19 +23,6 @@ class DataBusComposerTests : public ::testing::Test {
 
     using Curve = curve::BN254;
     using FF = Curve::ScalarField;
-    using Point = Curve::AffineElement;
-    using CommitmentKey = bb::CommitmentKey<Curve>;
-
-    /**
-     * @brief Generate a simple test circuit that includes arithmetic and goblin ecc op gates
-     *
-     * @param builder
-     */
-    void generate_test_circuit(auto& builder)
-    {
-        // Add some ecc op gates and arithmetic gates
-        GoblinMockCircuits::construct_simple_circuit(builder);
-    }
 };
 
 /**
@@ -49,39 +36,36 @@ TEST_F(DataBusComposerTests, CallDataRead)
 {
     auto op_queue = std::make_shared<bb::ECCOpQueue>();
 
-    // Add mock data to op queue to simulate interaction with a previous circuit
-    GoblinMockCircuits::perform_op_queue_interactions_for_mock_first_circuit(op_queue);
-
     auto builder = GoblinUltraCircuitBuilder{ op_queue };
 
-    // Create a general test circuit
-    generate_test_circuit(builder);
+    // Add some ecc op gates and arithmetic gates
+    GoblinMockCircuits::construct_simple_circuit(builder);
 
-    // Add some values to calldata and store the corresponding witness index
-    std::array<FF, 5> calldata_values = { 7, 10, 3, 12, 1 };
+    // Add some values to calldata
+    std::vector<FF> calldata_values = { 7, 10, 3, 12, 1 };
     for (auto& val : calldata_values) {
         builder.add_public_calldata(val);
     }
 
-    // Define some indices at which to read calldata
-    std::array<uint32_t, 2> read_index_values = { 1, 4 };
+    // Define some raw indices at which to read calldata (these will be ASSERTed to be valid)
+    std::vector<uint32_t> read_indices = { 1, 4 };
 
-    // Create some calldata lookup gates
-    for (uint32_t& read_index : read_index_values) {
+    // Create some calldata read gates. (Normally we'd use the result of the read. Example of that is below)
+    for (uint32_t& read_idx : read_indices) {
         // Create a variable corresponding to the index at which we want to read into calldata
-        uint32_t read_idx_witness_idx = builder.add_variable(read_index);
+        uint32_t read_idx_witness_idx = builder.add_variable(read_idx);
 
-        // Create a variable corresponding to the result of the read. Note that we do not in general connect reads from
-        // calldata via copy constraints (i.e. we create a unique variable for the result of each read)
-        ASSERT_LT(read_index, builder.public_calldata.size());
-        FF calldata_value = builder.get_variable(builder.public_calldata[read_index]);
-        uint32_t value_witness_idx = builder.add_variable(calldata_value);
-
-        builder.create_calldata_lookup_gate({ read_idx_witness_idx, value_witness_idx });
-        // TODO(https://github.com/AztecProtocol/barretenberg/issues/821): automate updating of read counts
-        builder.calldata_read_counts[read_index]++;
+        builder.read_calldata(read_idx_witness_idx);
     }
 
+    // In general we'll want to use the result of a calldata read in another operation. Here's an example using
+    // an add gate to show that the result of the read is as expected:
+    uint32_t read_idx = 2;
+    FF expected_result = calldata_values[read_idx];
+    uint32_t read_idx_witness_idx = builder.add_variable(read_idx);
+    uint32_t result_witness_idx = builder.read_calldata(read_idx_witness_idx);
+    builder.create_add_gate({ result_witness_idx, builder.zero_idx, builder.zero_idx, 1, 0, 0, -expected_result });
+
     // Construct and verify Honk proof
     auto instance = std::make_shared<ProverInstance_<GoblinUltraFlavor>>(builder);
     // For debugging, use "instance_inspector::print_databus_info(instance)"