Public input permutation

cpp/src/aztec/common/assert.hpp

@@ -10,8 +10,12 @@
         true ? static_cast<void>(0) : static_cast<void>((expression));                                                 \
     }
 
+// NOLINTBEGIN
+
 #if NDEBUG
 #define ASSERT(expression) DONT_EVALUATE((expression))
 #else
 #define ASSERT(expression) assert((expression))
 #endif // NDEBUG
+
+// NOLINTEND

cpp/src/aztec/honk/circuit_constructors/circuit_constructor_base.hpp

@@ -1,5 +1,6 @@
 #pragma once
 #include <ecc/curves/bn254/fr.hpp>
+#include <utility>
 
 namespace honk {
 static constexpr uint32_t DUMMY_TAG = 0;
@@ -98,55 +99,60 @@ template <size_t program_width_> class CircuitConstructorBase {
   public:
     static constexpr size_t program_width = program_width_;
     std::vector<std::string> selector_names_;
-    size_t n; // the circuit size; we should rename
+    size_t num_gates = 0;
+    // TODO(Adrian): It would be better to store an array of size program_width_
+    // to make the composer agnostic of the wire name.
     std::vector<uint32_t> w_l;
     std::vector<uint32_t> w_r;
     std::vector<uint32_t> w_o;
     std::vector<uint32_t> w_4;
     std::vector<uint32_t> public_inputs;
     std::vector<barretenberg::fr> variables;
-    std::vector<uint32_t> next_var_index; // index of next variable in equivalence class (=REAL_VARIABLE if you're last)
-    std::vector<uint32_t>
-        prev_var_index; // index of  previous variable in equivalence class (=FIRST if you're in a cycle alone)
-    std::vector<uint32_t> real_variable_index; // indices of corresponding real variables
+    // index of next variable in equivalence class (=REAL_VARIABLE if you're last)
+    std::vector<uint32_t> next_var_index;
+    // index of  previous variable in equivalence class (=FIRST if you're in a cycle alone)
+    std::vector<uint32_t> prev_var_index;
+    // indices of corresponding real variables
+    std::vector<uint32_t> real_variable_index;
     std::vector<uint32_t> real_variable_tags;
     uint32_t current_tag = DUMMY_TAG;
-    std::map<uint32_t, uint32_t>
-        tau; // The permutation on variable tags. See
-             // https://github.com/AztecProtocol/plonk-with-lookups-private/blob/new-stuff/GenPermuations.pdf
-             // DOCTODO: replace with the relevant wiki link.
+    // The permutation on variable tags. See
+    // https://github.com/AztecProtocol/plonk-with-lookups-private/blob/new-stuff/GenPermuations.pdf
+    // DOCTODO: replace with the relevant wiki link.
+    std::map<uint32_t, uint32_t> tau;
 
     size_t num_selectors;
     std::vector<std::vector<barretenberg::fr>> selectors;
-    numeric::random::Engine* rand_engine;
+    numeric::random::Engine* rand_engine = nullptr;
     bool _failed = false;
     std::string _err;
     static constexpr uint32_t REAL_VARIABLE = UINT32_MAX - 1;
     static constexpr uint32_t FIRST_VARIABLE_IN_CLASS = UINT32_MAX - 2;
-    static constexpr size_t NUM_RESERVED_GATES = 4; // this must be >= num_roots_cut_out_of_vanishing_polynomial
 
     // Enum values spaced in increments of 30-bits (multiples of 2 ** 30).
-    enum WireType { LEFT = 0U, RIGHT = (1U << 30U), OUTPUT = (1U << 31U), FOURTH = 0xc0000000 };
+    // TODO(Adrian): This is unused, and this type of hard coded data should be avoided
+    // enum WireType { LEFT = 0U, RIGHT = (1U << 30U), OUTPUT = (1U << 31U), FOURTH = 0xc0000000 };
 
     CircuitConstructorBase(std::vector<std::string> selector_names, size_t num_selectors = 0, size_t size_hint = 0)
-        : selector_names_(selector_names)
-        , n(0)
+        : selector_names_(std::move(selector_names))
         , num_selectors(num_selectors)
         , selectors(num_selectors)
-        , rand_engine(nullptr)
     {
         for (auto& p : selectors) {
             p.reserve(size_hint);
         }
     }
 
-    CircuitConstructorBase(CircuitConstructorBase&& other) = default;
-    CircuitConstructorBase& operator=(CircuitConstructorBase&& other) = default;
-    virtual ~CircuitConstructorBase(){};
+    CircuitConstructorBase(const CircuitConstructorBase& other) = delete;
+    CircuitConstructorBase(CircuitConstructorBase&& other) noexcept = default;
+    CircuitConstructorBase& operator=(const CircuitConstructorBase& other) = delete;
+    CircuitConstructorBase& operator=(CircuitConstructorBase&& other) noexcept = default;
+    virtual ~CircuitConstructorBase() = default;
 
-    virtual size_t get_num_gates() const { return n; }
-    virtual void print_num_gates() const { std::cout << n << std::endl; }
+    virtual size_t get_num_gates() const { return num_gates; }
+    virtual void print_num_gates() const { std::cout << num_gates << std::endl; }
     virtual size_t get_num_variables() const { return variables.size(); }
+    // TODO(Adrian): Feels wrong to let the zero_idx be changed.
     uint32_t zero_idx = 0;
 
     virtual void create_add_gate(const add_triple& in) = 0;
@@ -264,24 +270,24 @@ template <size_t program_width_> class CircuitConstructorBase {
      * */
     virtual void set_public_input(const uint32_t witness_index)
     {
-        bool does_not_exist = true;
-        for (size_t i = 0; i < public_inputs.size(); ++i) {
-            does_not_exist = does_not_exist && (public_inputs[i] != witness_index);
-        }
-        if (does_not_exist) {
-            public_inputs.emplace_back(witness_index);
-        }
-        ASSERT(does_not_exist);
-        if (!does_not_exist && !failed()) {
-            failure("Attempted to set a public input that is already public!");
+        for (const uint32_t public_input : public_inputs) {
+            if (public_input == witness_index) {
+                if (!failed()) {
+                    failure("Attempted to set a public input that is already public!");
+                }
+                return;
+            }
         }
+        public_inputs.emplace_back(witness_index);
     }
 
     virtual void assert_equal(const uint32_t a_idx, const uint32_t b_idx, std::string const& msg = "assert_equal");
 
-    size_t get_circuit_subgroup_size(const size_t num_gates)
+    // TODO(Adrian): This method should belong in the ComposerHelper, where the number of reserved gates can be
+    // correctly set
+    size_t get_circuit_subgroup_size(const size_t num_gates) const
     {
-        size_t log2_n = static_cast<size_t>(numeric::get_msb(num_gates));
+        auto log2_n = static_cast<size_t>(numeric::get_msb(num_gates));
         if ((1UL << log2_n) != (num_gates)) {
             ++log2_n;
         }
@@ -299,19 +305,16 @@ template <size_t program_width_> class CircuitConstructorBase {
     // uint32::MAX number of variables
     void assert_valid_variables(const std::vector<uint32_t>& variable_indices)
     {
-        for (size_t i = 0; i < variable_indices.size(); i++) {
-            ASSERT(is_valid_variable(variable_indices[i]));
+        for (const auto& variable_index : variable_indices) {
+            ASSERT(is_valid_variable(variable_index));
         }
     }
-    bool is_valid_variable(uint32_t variable_index)
-    {
-        return static_cast<uint32_t>(variables.size()) > variable_index;
-    };
+    bool is_valid_variable(uint32_t variable_index) { return variable_index < variables.size(); };
 
     bool failed() const { return _failed; };
     const std::string& err() const { return _err; };
 
-    void set_err(std::string msg) { _err = msg; }
+    void set_err(std::string msg) { _err = std::move(msg); }
     void failure(std::string msg)
     {
         _failed = true;

cpp/src/aztec/honk/circuit_constructors/standard_circuit_constructor.cpp

@@ -32,7 +32,7 @@ void StandardCircuitConstructor::create_add_gate(const add_triple& in)
     q_3.emplace_back(in.c_scaling);
     q_c.emplace_back(in.const_scaling);
 
-    ++n;
+    ++num_gates;
 }
 
 /**
@@ -85,7 +85,7 @@ void StandardCircuitConstructor::create_balanced_add_gate(const add_quad& in)
     q_3.emplace_back(fr::neg_one());
     q_c.emplace_back(fr::zero());
 
-    ++n;
+    ++num_gates;
 
     w_l.emplace_back(temp_idx);
     w_r.emplace_back(in.c);
@@ -96,7 +96,7 @@ void StandardCircuitConstructor::create_balanced_add_gate(const add_quad& in)
     q_3.emplace_back(in.d_scaling);
     q_c.emplace_back(in.const_scaling);
 
-    ++n;
+    ++num_gates;
 
     // in.d must be between 0 and 3
     // i.e. in.d * (in.d - 1) * (in.d - 2) = 0
@@ -111,7 +111,7 @@ void StandardCircuitConstructor::create_balanced_add_gate(const add_quad& in)
     q_3.emplace_back(fr::neg_one());
     q_c.emplace_back(fr::zero());
 
-    ++n;
+    ++num_gates;
 
     constexpr fr neg_two = -fr(2);
     w_l.emplace_back(temp_2_idx);
@@ -123,7 +123,7 @@ void StandardCircuitConstructor::create_balanced_add_gate(const add_quad& in)
     q_3.emplace_back(fr::zero());
     q_c.emplace_back(fr::zero());
 
-    ++n;
+    ++num_gates;
 }
 
 void StandardCircuitConstructor::create_big_add_gate_with_bit_extraction(const add_quad& in)
@@ -201,7 +201,7 @@ void StandardCircuitConstructor::create_mul_gate(const mul_triple& in)
     q_3.emplace_back(in.c_scaling);
     q_c.emplace_back(in.const_scaling);
 
-    ++n;
+    ++num_gates;
 }
 
 /**
@@ -225,7 +225,7 @@ void StandardCircuitConstructor::create_bool_gate(const uint32_t variable_index)
     q_3.emplace_back(fr::neg_one());
     q_c.emplace_back(fr::zero());
 
-    ++n;
+    ++num_gates;
 }
 
 /**
@@ -247,7 +247,7 @@ void StandardCircuitConstructor::create_poly_gate(const poly_triple& in)
     q_3.emplace_back(in.q_o);
     q_c.emplace_back(in.q_c);
 
-    ++n;
+    ++num_gates;
 }
 
 void StandardCircuitConstructor::create_fixed_group_add_gate_with_init(const fixed_group_add_quad& in,
@@ -687,7 +687,7 @@ void StandardCircuitConstructor::fix_witness(const uint32_t witness_index, const
     q_2.emplace_back(fr::zero());
     q_3.emplace_back(fr::zero());
     q_c.emplace_back(-witness_value);
-    ++n;
+    ++num_gates;
 }
 
 uint32_t StandardCircuitConstructor::put_constant_variable(const barretenberg::fr& variable)
@@ -738,7 +738,7 @@ bool StandardCircuitConstructor::check_circuit()
 
     fr gate_sum;
     fr left, right, output;
-    for (size_t i = 0; i < n; i++) {
+    for (size_t i = 0; i < num_gates; i++) {
         gate_sum = fr::zero();
         left = get_variable(w_l[i]);
         right = get_variable(w_r[i]);

cpp/src/aztec/honk/circuit_constructors/standard_circuit_constructor.hpp

@@ -19,6 +19,7 @@ class StandardCircuitConstructor : public CircuitConstructorBase<STANDARD_HONK_W
 
     // These are variables that we have used a gate on, to enforce that they are
     // equal to a defined value.
+    // TODO(Adrian): Why is this not in CircuitConstructorBase
     std::map<barretenberg::fr, uint32_t> constant_variable_indices;
 
     StandardCircuitConstructor(const size_t size_hint = 0)
@@ -29,12 +30,15 @@ class StandardCircuitConstructor : public CircuitConstructorBase<STANDARD_HONK_W
         w_o.reserve(size_hint);
         // To effieciently constrain wires to zero, we set the first value of w_1 to be 0, and use copy constraints for
         // all future zero values.
+        // TODO(Adrian): This should be done in a constant way, maybe by initializing the constant_variable_indices map
         zero_idx = put_constant_variable(barretenberg::fr::zero());
     };
 
+    StandardCircuitConstructor(const StandardCircuitConstructor& other) = delete;
     StandardCircuitConstructor(StandardCircuitConstructor&& other) = default;
+    StandardCircuitConstructor& operator=(const StandardCircuitConstructor& other) = delete;
     StandardCircuitConstructor& operator=(StandardCircuitConstructor&& other) = default;
-    ~StandardCircuitConstructor() {}
+    ~StandardCircuitConstructor() override = default;
 
     void assert_equal_constant(uint32_t const a_idx,
                                barretenberg::fr const& b,
@@ -54,6 +58,7 @@ class StandardCircuitConstructor : public CircuitConstructorBase<STANDARD_HONK_W
 
     fixed_group_add_quad previous_add_quad;
 
+    // TODO(Adrian): This should be a virtual overridable method in the base class.
     void fix_witness(const uint32_t witness_index, const barretenberg::fr& witness_value);
 
     std::vector<uint32_t> decompose_into_base4_accumulators(const uint32_t witness_index,
@@ -74,6 +79,7 @@ class StandardCircuitConstructor : public CircuitConstructorBase<STANDARD_HONK_W
     accumulator_triple create_and_constraint(const uint32_t a, const uint32_t b, const size_t num_bits);
     accumulator_triple create_xor_constraint(const uint32_t a, const uint32_t b, const size_t num_bits);
 
+    // TODO(Adrian): The 2 following methods should be virtual in the base class
     uint32_t put_constant_variable(const barretenberg::fr& variable);
 
     size_t get_num_constant_gates() const override { return 0; }