feat: consistent pedersen hash (work in progress) (#1945)

This PR implements part of our pedersen refactor project
https://hackmd.io/XYBiWhHPT9C1bo4nrtoo0A?view

We introduce a new stdlib class `cycle_group` , that implements a full
suite of group operations over a generic SNARK-friendly embedded curve.

Of key interest is `cycle_group::batch_mul`, which uses both fixed-base
and variable-base multiplication to optimally evaluate in-circuit scalar
multiplications. All external `cycle_group` operations are statistically
complete i.e. the edge-cases for point addition on short weierstrass
curves are handled, either explicitly or statistically using 'offset
generators' (i.e. when performing a cycle_group computation, precomputed
generator points are introduced to prevent intermediate results
triggering addition formula edge-cases).

This enables us to efficiently represent points at infinity. In the
future we can reduce the complexity of our stdlib/recursion
implementation by not requiring Prover commitments to not be points at
infinity.

Additionally, `pedersen_commitment` and `pedersen_hash` have been
refactored according to the project specification - using `cycle_group`
methods internally instead of bespoke algorithms that are difficult to
reproduce.

This PR does not modify existing interfaces or implementations w.r.t
pedersen commitents/hashing. This will come as part 2 of the refactor as
the interface modifications increase would increase the code surface of
an already large PR.


# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [x] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: Charlie Lye <[email protected]>

barretenberg/cpp/src/barretenberg/benchmark/relations_bench/CMakeLists.txt

@@ -7,6 +7,7 @@ relations.bench.cpp
 # Required libraries for benchmark suites
 set(LINKED_LIBRARIES
   polynomials
+  proof_system
   benchmark::benchmark
 )
 

barretenberg/cpp/src/barretenberg/crypto/generators/fixed_base_scalar_mul.hpp

@@ -1,4 +1,7 @@
 #pragma once
+
+// TODO(@zac-williamson #2341 delete this file once we migrate to new pedersen hash standard)
+
 #include "./generator_data.hpp"
 #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
 

barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.cpp

@@ -1,5 +1,7 @@
 #include "./generator_data.hpp"
 
+// TODO(@zac-williamson #2341 delete this file once we migrate to new pedersen hash standard)
+
 namespace crypto {
 namespace generators {
 namespace {

barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.hpp

@@ -1,4 +1,6 @@
 #pragma once
+
+// TODO(@zac-williamson #2341 delete this file once we migrate to new pedersen hash standard)
 #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
 #include <array>
 #include <tuple>

barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.test.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-williamson #2341 delete this file once we migrate to new pedersen hash standard)
+
 #include "./generator_data.hpp"
 #include "./fixed_base_scalar_mul.hpp"
 #include "barretenberg/common/streams.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file and rename c_bind_new to c_bind once we have migrated to new hash standard
+
 #include "c_bind.hpp"
 #include "barretenberg/common/mem.hpp"
 #include "barretenberg/common/serialize.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/c_bind.hpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file and rename c_bind_new to c_bind once we have migrated to new hash standard
+
 #pragma once
 #include "barretenberg/common/mem.hpp"
 #include "barretenberg/common/serialize.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "./pedersen.hpp"
 #include "./convert_buffer_to_field.hpp"
 #include "barretenberg/common/throw_or_abort.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen.hpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #pragma once
 #include "../generators/fixed_base_scalar_mul.hpp"
 #include "../generators/generator_data.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_lookup.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "./pedersen_lookup.hpp"
 #include "../pedersen_hash/pedersen_lookup.hpp"
 #include "./convert_buffer_to_field.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_lookup.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
 
 namespace crypto {

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_lookup.test.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "barretenberg/numeric/bitop/get_msb.hpp"
 #include "barretenberg/numeric/random/engine.hpp"
 #include <gtest/gtest.h>

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_refactor.cpp

@@ -0,0 +1,51 @@
+// TODO(@zac-wiliamson #2341 rename to pedersen.cpp once we migrate to new hash standard)
+
+#include "./pedersen_refactor.hpp"
+#include "./convert_buffer_to_field.hpp"
+#include "barretenberg/common/serialize.hpp"
+#include "barretenberg/common/throw_or_abort.hpp"
+#include <iostream>
+#ifndef NO_OMP_MULTITHREADING
+#include <omp.h>
+#endif
+
+namespace crypto {
+
+/**
+ * @brief Given a vector of fields, generate a pedersen commitment using the indexed generators.
+ *
+ * @details This method uses `Curve::BaseField` members as inputs. This aligns with what we expect when creating
+ * grumpkin commitments to field elements inside a BN254 SNARK circuit.
+ *
+ * @note Fq is the *coordinate field* of Curve. Curve itself is a SNARK-friendly curve,
+ * i.e. Fq represents the native field type of the SNARK circuit.
+ * @param inputs
+ * @param hash_index
+ * @param generator_context
+ * @return Curve::AffineElement
+ */
+template <typename Curve>
+typename Curve::AffineElement pedersen_commitment_refactor<Curve>::commit_native(
+    const std::vector<Fq>& inputs, const size_t hash_index, const generator_data<Curve>* const generator_context)
+{
+    const auto generators = generator_context->conditional_extend(inputs.size() + hash_index);
+    Element result = Group::point_at_infinity;
+
+    // `Curve::Fq` represents the field that `Curve` is defined over (i.e. x/y coordinate field) and `Curve::Fr` is the
+    // field whose modulus = the group order of `Curve`.
+    // The `Curve` we're working over here is a generic SNARK-friendly curve. i.e. the SNARK circuit is defined over a
+    // field equivalent to `Curve::Fq`. This adds complexity when we wish to commit to SNARK circuit field elements, as
+    // these are members of `Fq` and *not* `Fr`. We cast to `uint256_t` in order to convert an element of `Fq` into an
+    // `Fr` element, which is the required type when performing scalar multiplications.
+    static_assert(Fr::modulus > Fq::modulus,
+                  "pedersen_commitment::commit_native Curve subgroup field is smaller than coordinate field. Cannot "
+                  "perform injective conversion");
+    for (size_t i = 0; i < inputs.size(); ++i) {
+        Fr scalar_multiplier(static_cast<uint256_t>(inputs[i]));
+        result += Element(generators.get(i, hash_index)) * scalar_multiplier;
+    }
+    return result;
+}
+
+template class pedersen_commitment_refactor<curve::Grumpkin>;
+} // namespace crypto

barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen_refactor.hpp

@@ -0,0 +1,109 @@
+#pragma once
+
+// TODO(@zac-wiliamson #2341 rename to pedersen.hpp once we migrate to new hash standard)
+
+#include "../generators/fixed_base_scalar_mul.hpp"
+#include "../generators/generator_data.hpp"
+#include "barretenberg/ecc/curves/bn254/bn254.hpp"
+#include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
+#include <array>
+
+namespace crypto {
+
+/**
+ * @brief Contains a vector of precomputed generator points.
+ *        Generators are defined via a domain separator.
+ *        Number of generators in generator_data is fixed for a given object instance.
+ *
+ * @details generator_data is used to precompute short lists of commonly used generators,
+ *          (e.g. static inline const default_generators = generator_data()).
+ *          If an algorithm requires more than `_size_ generators,
+ *          the `conditional_extend` method can be called to return a new `generator_data` object.
+ *          N.B. we explicitly do not support mutating an existing `generator_data` object to increase the size of
+ *          its `std::vector<affine_element> generators` member variable.
+ *          This is because this class is intended to be used as a `static` member of other classes to provide lists
+ * of precomputed generators. Mutating static member variables is *not* thread safe!
+ */
+template <typename Curve> class generator_data {
+  public:
+    using Group = typename Curve::Group;
+    using AffineElement = typename Curve::AffineElement;
+    static inline constexpr size_t DEFAULT_NUM_GENERATORS = 32;
+    static inline const std::string DEFAULT_DOMAIN_SEPARATOR = "default_domain_separator";
+    inline generator_data(const size_t num_generators = DEFAULT_NUM_GENERATORS,
+                          const std::string& domain_separator = DEFAULT_DOMAIN_SEPARATOR)
+        : _domain_separator(domain_separator)
+        , _domain_separator_bytes(domain_separator.begin(), domain_separator.end())
+        , _size(num_generators){};
+
+    [[nodiscard]] inline std::string domain_separator() const { return _domain_separator; }
+    [[nodiscard]] inline size_t size() const { return _size; }
+    [[nodiscard]] inline AffineElement get(const size_t index, const size_t offset = 0) const
+    {
+        ASSERT(index + offset <= _size);
+        return generators[index + offset];
+    }
+
+    /**
+     * @brief If more generators than `_size` are required, this method will return a new `generator_data` object
+     *        with the required generators.
+     *
+     * @note Question: is this a good pattern to support? Ideally downstream code would ensure their
+     *       `generator_data` object is sufficiently large to cover potential needs.
+     *       But if we did not support this pattern, it would make downstream code more complex as each method that
+     *       uses `generator_data` would have to perform this accounting logic.
+     *
+     * @param target_num_generators
+     * @return generator_data
+     */
+    [[nodiscard]] inline generator_data conditional_extend(const size_t target_num_generators) const
+    {
+        if (target_num_generators <= _size) {
+            return *this;
+        }
+        return { target_num_generators, _domain_separator };
+    }
+
+  private:
+    std::string _domain_separator;
+    std::vector<uint8_t> _domain_separator_bytes;
+    size_t _size;
+    // ordering of static variable initialization is undefined, so we make `default_generators` private
+    // and only accessible via `get_default_generators()`, which ensures var will be initialized at the cost of some
+    // small runtime checks
+    inline static const generator_data default_generators =
+        generator_data(generator_data::DEFAULT_NUM_GENERATORS, generator_data::DEFAULT_DOMAIN_SEPARATOR);
+
+  public:
+    inline static const generator_data* get_default_generators() { return &default_generators; }
+    const std::vector<AffineElement> generators = (Group::derive_generators_secure(_domain_separator_bytes, _size));
+};
+
+template class generator_data<curve::Grumpkin>;
+
+/**
+ * @brief Performs pedersen commitments!
+ *
+ * To commit to a size-n list of field elements `x`, a commitment is defined as:
+ *
+ *      Commit(x) = x[0].g[0] + x[1].g[1] + ... + x[n-1].g[n-1]
+ *
+ * Where `g` is a list of generator points defined by `generator_data`
+ *
+ */
+template <typename Curve> class pedersen_commitment_refactor {
+  public:
+    using AffineElement = typename Curve::AffineElement;
+    using Element = typename Curve::Element;
+    using Fr = typename Curve::ScalarField;
+    using Fq = typename Curve::BaseField;
+    using Group = typename Curve::Group;
+
+    static AffineElement commit_native(
+        const std::vector<Fq>& inputs,
+        size_t hash_index = 0,
+        const generator_data<Curve>* generator_context = generator_data<Curve>::get_default_generators());
+};
+
+extern template class pedersen_commitment_refactor<curve::Grumpkin>;
+} // namespace crypto

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/c_bind.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file and rename c_bind_new to c_bind once we have migrated to new hash standard
+
 #include "barretenberg/common/mem.hpp"
 #include "barretenberg/common/serialize.hpp"
 #include "barretenberg/common/streams.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/c_bind.hpp

@@ -1,4 +1,6 @@
 #pragma once
+// TODO(@zac-wiliamson #2341 delete this file and rename c_bind_new to c_bind once we have migrated to new hash standard
+
 #include "barretenberg/common/wasm_export.hpp"
 #include "barretenberg/ecc/curves/bn254/fr.hpp"
 

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "./pedersen.hpp"
 #include <iostream>
 #ifndef NO_OMP_MULTITHREADING

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.hpp

@@ -1,4 +1,7 @@
 #pragma once
+
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "../generators/fixed_base_scalar_mul.hpp"
 #include "../generators/generator_data.hpp"
 #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen_lookup.cpp

@@ -1,3 +1,5 @@
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
+
 #include "./pedersen_lookup.hpp"
 
 #include <mutex>

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen_lookup.hpp

@@ -1,4 +1,5 @@
 #pragma once
+// TODO(@zac-wiliamson #2341 delete this file once we migrate to new hash standard
 
 #include "barretenberg/ecc/curves/grumpkin/grumpkin.hpp"
 

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen_refactor.cpp

@@ -0,0 +1,57 @@
+#include "./pedersen_refactor.hpp"
+#include <iostream>
+#ifndef NO_OMP_MULTITHREADING
+#include <omp.h>
+#endif
+
+// TODO(@zac-wiliamson #2341 rename to pedersen.cpp once we migrate to new hash standard)
+
+namespace crypto {
+
+using namespace generators;
+
+/**
+ * Given a vector of fields, generate a pedersen hash using the indexed generators.
+ */
+
+/**
+ * @brief Given a vector of fields, generate a pedersen hash using generators from `generator_context`.
+ *
+ * @details `hash_index` is used to access offset elements of `generator_context` if required.
+ *          e.g. if one desires to compute
+ *          `inputs[0] * [generators[hash_index]] + `inputs[1] * [generators[hash_index + 1]]` + ... etc
+ *          Potentially useful to ensure multiple hashes with the same domain separator cannot collide.
+ *
+ * TODO(@suyash67) can we change downstream code so that `hash_index` is no longer required? Now we have a proper
+ * domain_separator parameter, we no longer need to specify different generator indices to ensure hashes cannot collide.
+ * @param inputs what are we hashing?
+ * @param hash_index Describes an offset into the list of generators, if required
+ * @param generator_context
+ * @return Fq (i.e. SNARK circuit scalar field, when hashing using a curve defined over the SNARK circuit scalar field)
+ */
+template <typename Curve>
+typename Curve::BaseField pedersen_hash_refactor<Curve>::hash_multiple(const std::vector<Fq>& inputs,
+                                                                       const size_t hash_index,
+                                                                       const generator_data* const generator_context)
+{
+    const auto generators = generator_context->conditional_extend(inputs.size() + hash_index);
+
+    Element result = get_length_generator() * Fr(inputs.size());
+
+    for (size_t i = 0; i < inputs.size(); ++i) {
+        result += generators.get(i, hash_index) * Fr(static_cast<uint256_t>(inputs[i]));
+    }
+    result = result.normalize();
+    return result.x;
+}
+
+template <typename Curve>
+typename Curve::BaseField pedersen_hash_refactor<Curve>::hash(const std::vector<Fq>& inputs,
+                                                              size_t hash_index,
+                                                              const generator_data* const generator_context)
+{
+    return hash_multiple(inputs, hash_index, generator_context);
+}
+
+template class pedersen_hash_refactor<curve::Grumpkin>;
+} // namespace crypto