chore: pedersen hash audit

barretenberg/cpp/scripts/audit/audit_scopes/pedersen_hash_audit_scope.md

@@ -1,14 +1,16 @@
 # Pedersen Hash Audit Scope
 
 Repository: https://github.com/AztecProtocol/aztec-packages
-Commit hash: 4a956ceb179c2fe855e4f1fd78f2594e7fc3f5ea
+Commit hash:
 
 ### Files to audit
 
 #### Native implementation
 1. ```barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.hpp```
 2. ```barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.cpp```
-3. ```barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.hpp```
+3. ```barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen.hpp```
+4. ```barretenberg/cpp/src/barretenberg/crypto/pedersen_commitment/pedersen.cpp```
+5. ```barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.hpp```
 
 ### Summary of the module
 
@@ -34,6 +36,7 @@ Key features:
 
 #### Tests
 - ```barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.test.cpp```
+- ```barretenberg/cpp/src/barretenberg/crypto/generators/generator_data.test.cpp```
 
 ### Documentation
 

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

@@ -62,12 +62,24 @@ template <typename Curve> class generator_data {
     static constexpr std::span<const AffineElement> precomputed_generators =
         get_precomputed_generators<Group, "DEFAULT_DOMAIN_SEPARATOR", DEFAULT_NUM_GENERATORS, 0>();
 
+    /**
+     * @brief Return a view of `num_generators` generators for a given `domain_separator`, starting at generator index
+     * `generator_offset`.
+     *
+     * @param num_generators number of generators to return
+     * @param generator_offset starting index of the generators to return
+     * @param domain_separator domain separator string
+     * @return GeneratorView
+     */
     [[nodiscard]] inline GeneratorView get(const size_t num_generators,
                                            const size_t generator_offset = 0,
                                            const std::string_view domain_separator = DEFAULT_DOMAIN_SEPARATOR) const
     {
         const bool is_default_domain = domain_separator == DEFAULT_DOMAIN_SEPARATOR;
-        if (is_default_domain && (num_generators + generator_offset) < DEFAULT_NUM_GENERATORS) {
+
+        // Case 1: we want default generators (is_default_domain), and the requested slice is contained within the
+        // precomputed array (num_generators + generator_offset <= DEFAULT_NUM_GENERATORS)
+        if (is_default_domain && (num_generators + generator_offset <= DEFAULT_NUM_GENERATORS)) {
             return GeneratorView{ precomputed_generators.data() + generator_offset, num_generators };
         }
 
@@ -84,7 +96,7 @@ template <typename Curve> class generator_data {
             initialized_precomputed_generators = true;
         }
 
-        // if the generator map does not contain our desired generators, add entry into map
+        // Case 3: the requested domain is not contained in the generator map---add entry into map.
         if (!map.contains(std::string(domain_separator))) {
             map.insert({
                 std::string(domain_separator),
@@ -93,8 +105,8 @@ template <typename Curve> class generator_data {
         }
 
         GeneratorList& generators = map.at(std::string(domain_separator));
-
-        // If the current GeneratorList does not contain enough generators, extend it
+        // Case 4: the requested domain exists in the generator map but does not contain enough points to satisfy the
+        // request---extend the generator list.
         if (num_generators + generator_offset > generators.size()) {
             const size_t num_extra_generators = num_generators + generator_offset - generators.size();
             GeneratorList extended_generators =
@@ -121,7 +133,7 @@ template <typename Curve> class generator_data {
     // the external functionality of `generator_data`.
     // i.e. `generator_data.get` will return the same output regardless of the internal state of `generator_data`.
 
-    // bool that describes whether we've copied the precomputed enerators into `generator_map`. This cannot be done at
+    // bool that describes whether we've copied the precomputed generators into `generator_map`. This cannot be done at
     // compile-time because std::map is a dynamically sized object.
     mutable bool initialized_precomputed_generators = false;
 

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

@@ -40,6 +40,65 @@ TEST(GeneratorContext, DeriveDefaultGenerators)
     for (size_t i = 0; i < default_generators.size(); ++i) {
         EXPECT_EQ(default_generators[i], expected_default_generators[i]);
     }
+
+    // Verifies that the hard-coded precomputed generators match the derivation
+    EXPECT_TRUE((bb::check_precomputed_generators<grumpkin::g1,
+                                                  bb::detail::DomainSeparator("DEFAULT_DOMAIN_SEPARATOR"),
+                                                  generator_data<curve::Grumpkin>::DEFAULT_NUM_GENERATORS>()));
+}
+
+// Tests generator_data<Grumpkin>::get() for different domain separators, num_generators, and offsets.
+TEST(GeneratorContext, GeneratorDataGetVariousCases)
+{
+    generator_data<curve::Grumpkin> generator_d;
+
+    constexpr size_t default_num_generators = generator_data<curve::Grumpkin>::DEFAULT_NUM_GENERATORS; // 8
+    constexpr std::string_view default_domain_separator =
+        generator_data<curve::Grumpkin>::DEFAULT_DOMAIN_SEPARATOR;         // "DEFAULT_DOMAIN_SEPARATOR"
+    constexpr std::string_view domain_separator = "TEST_DOMAIN_SEPARATOR"; // custom non-default domain
+
+    // Choose (n, offset) pairs that hit reach various branches in get(): (1) n+offset < 8, (2) n+offset == 8,
+    // (3)n+offset > 8
+    struct Case {
+        size_t num_generators;
+        size_t offset;
+    };
+    constexpr std::array<Case, 3> cases{ { { 5, 0 }, { 6, 2 }, { 9, 1 } } };
+
+    // Test both default and non-default domain separators
+    constexpr std::array<std::string_view, 2> domains{ { default_domain_separator, domain_separator } };
+
+    for (auto domain : domains) {
+        for (auto test_case : cases) {
+            const size_t num_generators = test_case.num_generators;
+            const size_t offset = test_case.offset;
+
+            // Compute generators using get()
+            auto generators = generator_d.get(num_generators, offset, domain);
+            ASSERT_EQ(generators.size(), num_generators);
+
+            auto derived_generators =
+                grumpkin::g1::derive_generators(domain, num_generators + offset, /*starting_index=*/0);
+            ASSERT_EQ(derived_generators.size(), num_generators + offset);
+
+            for (size_t i = 0; i < num_generators; ++i) {
+                EXPECT_EQ(generators[i], derived_generators[offset + i]);
+            }
+
+            const bool is_default_domain = (domain == default_domain_separator);
+
+            // For default-domain and num_generators + offset <= default_num_generators, the values must match
+            // precomputed_generators.
+            if (is_default_domain && (num_generators + offset <= default_num_generators)) {
+                auto default_generators = generator_data<curve::Grumpkin>::precomputed_generators;
+                ASSERT_GE(default_generators.size(), num_generators + offset);
+
+                for (size_t i = 0; i < num_generators; ++i) {
+                    EXPECT_EQ(generators[i], default_generators[offset + i]);
+                }
+            }
+        }
+    }
 }
 
 } // namespace bb::crypto

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

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

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/README.md

@@ -0,0 +1,35 @@
+# Pedersen Hash and Commitment
+
+This module implements the Pedersen hash (`crypto/pedersen_hash`) and commitment (`crypto/pedersen_commitment`) over the Grumpkin curve.
+
+## Pederson Commitment
+For a vector of field elements $x = [x_0, \dots, x_{n-1}]$, the commitment is $Commit(x) = \sum_{i=0}^{n-1} x_i \cdot G_i$, where $G = [G_0, G_1, \dots]$ are generators derived via `generator_data`.
+
+## Pederson Hash
+For a vector of field elements $v = [v_0, \ldots, v_{n-1}]$, the hash is defined as the x-coordinate of $Hash(v) = n · H_{len} + Commit(v)$, where $H_{len}$ is a length generator derived with domain separator `"pedersen_hash_length"`.
+
+The length term avoids trivial collisions arising from the `x(P) = x(-P)` symmetry and to bind the hash to the input length.
+
+## API
+### Hash
+- `hash(const std::vector<Fq>& inputs, const GeneratorContext context)`
+  - Hashes a vector of field elements using generators from `context`.
+
+- `hash_buffer(const std::vector<uint8_t>& input, const GeneratorContext context)`
+  - Converts an arbitrary byte buffer into field elements by splitting into 31-byte chunks, followed by hashing.
+  - For >2 chunks it hashes iteratively: `r = hash(e0,e1); r = hash(r,e2); ...`.
+
+### Commitment
+- `commit_native(const std::vector<Fq>& inputs, GeneratorContext context)`
+  - Computes $Commit(inputs)$ using generators from `context`.
+
+## Tests
+### Hash tests
+- `DeriveLengthGenerator`: verifies that the `"pedersen_hash_length"` generator matches a known point.
+- `Hash`: regression test for `hash({1,1})`.
+- `HashWithIndex`: regression test for `hash({1,1}, offset=5)`.
+- `Hash32Bytes`: checks iterative hashing performed within `hash_buffer` on a 32=31+1 byte input.
+
+### Commitment tests
+- `Commitment`: regression test for `commit_native({1,1})`.
+- `CommitmentWithZero`: regression test for `commit_native({0,1})`.

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

@@ -8,6 +8,7 @@ namespace bb::crypto {
 
 using bb::fr;
 
+// Verifies the domain-seperated "pedersen_hash_length" generator matches the expected
 TEST(Pedersen, DeriveLengthGenerator)
 {
     auto generator = pedersen_hash::length_generator;
@@ -18,18 +19,51 @@ TEST(Pedersen, DeriveLengthGenerator)
                   fr(uint256_t("0x2ecd88d15967bc53b885912e0d16866154acb6aac2d3f85e27ca7eefb2c19083"))));
 }
 
+// Verifies that hashing {1, 1} produces the expected result
 TEST(Pedersen, Hash)
 {
     auto x = pedersen_hash::Fq::one();
     auto r = pedersen_hash::hash({ x, x });
     EXPECT_EQ(r, fr(uint256_t("07ebfbf4df29888c6cd6dca13d4bb9d1a923013ddbbcbdc3378ab8845463297b")));
 }
 
+// Verifies that hashing {1, 1} with a generator-offset/context variant produces the expected result
 TEST(Pedersen, HashWithIndex)
 {
     auto x = pedersen_hash::Fq::one();
     auto r = pedersen_hash::hash({ x, x }, 5);
     EXPECT_EQ(r, fr(uint256_t("1c446df60816b897cda124524e6b03f36df0cec333fad87617aab70d7861daa6")));
 }
 
-} // namespace bb::crypto
+// Verifies that hashing a 32-byte buffer is equivalent to hashing two field elements via the intended chaining
+TEST(Pedersen, Hash32Bytes)
+{
+    using Fq = pedersen_hash::Fq;
+
+    std::vector<uint8_t> buf(32);
+    for (size_t i = 0; i < buf.size(); ++i) {
+        buf[i] = static_cast<uint8_t>(0xA0 + i);
+    }
+
+    // First 31-byte chunk
+    uint256_t acc0(0);
+    for (size_t i = 0; i < 31; ++i) {
+        acc0 = (acc0 << uint256_t(8));
+        acc0 += uint256_t(buf[i]);
+    }
+    Fq element0(acc0);
+
+    // Last 1-byte chunk
+    uint256_t acc1(0);
+    acc1 = (acc1 << uint256_t(8));
+    acc1 += uint256_t(buf[31]);
+    Fq element1(acc1);
+
+    // For exactly 2 elements, hash_buffer should equal hash({element0, element1})
+    auto expected = pedersen_hash::hash({ element0, element1 });
+    auto got = pedersen_hash::hash_buffer(buf);
+
+    EXPECT_EQ(got, expected);
+}
+
+} // namespace bb::crypto