Start to fix findings from Dmitry's review

bindings/java/build.gradle

@@ -2,7 +2,7 @@ plugins {
     id "application"
     id "java-test-fixtures"
     id "me.champeau.jmh" version "0.7.0"
-    id "com.diffplug.spotless" version "6.17.0"
+    id "com.diffplug.spotless" version "6.25.0"
 }
 
 repositories {
@@ -45,4 +45,4 @@ spotless {
 
 test {
     useJUnitPlatform()
-}
+}

bindings/java/src/main/java/ethereum/ckzg4844/CKZG4844JNI.java

@@ -47,28 +47,40 @@ public static void loadNativeLibrary() {
   public static final BigInteger BLS_MODULUS =
       new BigInteger(
           "52435875175126190479447740508185965837690552500527637822603658699938581184513");
+
   /** The number of bytes in a g1 point. */
   protected static final int BYTES_PER_G1 = 48;
+
   /** The number of bytes in a g2 point. */
   protected static final int BYTES_PER_G2 = 96;
+
   /** The number of bytes in a BLS scalar field element. */
   public static final int BYTES_PER_FIELD_ELEMENT = 32;
+
   /** The number of bits in a BLS scalar field element. */
   protected static final int BITS_PER_FIELD_ELEMENT = 255;
+
   /** The number of field elements in a blob. */
   public static final int FIELD_ELEMENTS_PER_BLOB = 4096;
+
   /** The number of field elements in an extended blob. */
   protected static final int FIELD_ELEMENTS_PER_EXT_BLOB = FIELD_ELEMENTS_PER_BLOB * 2;
+
   /** The number of field elements in a cell. */
   public static final int FIELD_ELEMENTS_PER_CELL = 64;
+
   /** The number of bytes in a KZG commitment. */
   public static final int BYTES_PER_COMMITMENT = 48;
+
   /** The number of bytes in a KZG proof. */
   public static final int BYTES_PER_PROOF = 48;
+
   /** The number of bytes in a blob. */
   public static final int BYTES_PER_BLOB = FIELD_ELEMENTS_PER_BLOB * BYTES_PER_FIELD_ELEMENT;
+
   /** The number of bytes in a single cell. */
   public static final int BYTES_PER_CELL = BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_CELL;
+
   /** The number of cells in an extended blob. */
   public static final int CELLS_PER_EXT_BLOB =
       FIELD_ELEMENTS_PER_EXT_BLOB / FIELD_ELEMENTS_PER_CELL;

bindings/python/README.md

@@ -7,7 +7,7 @@ This directory contains Python bindings for the C-KZG-4844 library.
 These bindings require `python3`, `PyYAML` and `make`.
 ```
 sudo apt install python3 python3-pip
-python3 -m pip install PyYAML
+python3 -m pip install build PyYAML
 ```
 
 ## Build & test

src/eip7594/cell.c

@@ -26,7 +26,7 @@
  */
 void print_cell(const Cell *cell) {
     for (size_t i = 0; i < FIELD_ELEMENTS_PER_CELL; i++) {
-        const Bytes32 *field = (const Bytes32 *)&cell->bytes[i * BYTES_PER_FIELD_ELEMENT];
-        print_bytes32(field);
+        const Bytes32 *field_element = (const Bytes32 *)&cell->bytes[i * BYTES_PER_FIELD_ELEMENT];
+        print_bytes32(field_element);
     }
 }

src/eip7594/eip7594.c

@@ -123,8 +123,8 @@ C_KZG_RET compute_cells_and_kzg_proofs(
         ret = new_g1_array(&proofs_g1, CELLS_PER_EXT_BLOB);
         if (ret != C_KZG_OK) goto out;
 
-        /* Compute the proofs, provide only the first half */
-        ret = compute_fk20_proofs(proofs_g1, poly_monomial, FIELD_ELEMENTS_PER_BLOB, s);
+        /* Compute the proofs, only uses the first half of the polynomial */
+        ret = compute_fk20_cell_proofs(proofs_g1, poly_monomial, s);
         if (ret != C_KZG_OK) goto out;
 
         /* Bit-reverse the proofs */
@@ -159,6 +159,7 @@ C_KZG_RET compute_cells_and_kzg_proofs(
  * @param[in]   num_cells           The number of available cells provided
  * @param[in]   s                   The trusted setup
  *
+ * @remark At least 50% of CELLS_PER_EXT_BLOB cells must be provided.
  * @remark Recovery is faster if there are fewer missing cells.
  * @remark If recovered_proofs is NULL, they will not be recomputed.
  */
@@ -259,10 +260,8 @@ C_KZG_RET recover_cells_and_kzg_proofs(
         );
         if (ret != C_KZG_OK) goto out;
 
-        /* Compute the proofs, provide only the first half */
-        ret = compute_fk20_proofs(
-            recovered_proofs_g1, recovered_cells_fr, FIELD_ELEMENTS_PER_BLOB, s
-        );
+        /* Compute the proofs, only uses the first half of the polynomial */
+        ret = compute_fk20_cell_proofs(recovered_proofs_g1, recovered_cells_fr, s);
         if (ret != C_KZG_OK) goto out;
 
         /* Bit-reverse the proofs */
@@ -517,9 +516,9 @@ static C_KZG_RET compute_weighted_sum_of_commitments(
  * This function computes `RLI = [sum_k r^k interpolation_poly_k(s)]` from the spec.
  *
  * @param[out]  commitment_out  Commitment to the aggregated interpolation poly
- * @param[in]   r_powers        Precomputed powers of the random challenge
- * @param[in]   cell_indices    Indices of the cells
- * @param[in]   cells           Array of cells
+ * @param[in]   r_powers        Precomputed powers of the random challenge, length `num_cells`
+ * @param[in]   cell_indices    Indices of the cells, length `num_cells`
+ * @param[in]   cells           Array of cells, length `num_cells`
  * @param[in]   num_cells       Number of cells
  * @param[in]   s               The trusted setup
  */
@@ -710,10 +709,9 @@ static C_KZG_RET computed_weighted_sum_of_proofs(
 
     for (uint64_t i = 0; i < num_cells; i++) {
         uint64_t pos = reverse_bits_limited(CELLS_PER_EXT_BLOB, cell_indices[i]);
-        fr_t coset_factor = s->roots_of_unity[pos];
-        // Compute h_k^n, with h_k and n as in the spec.
-        fr_pow(&coset_factor_pow, &coset_factor, FIELD_ELEMENTS_PER_CELL);
-        // Scale the power of r by h_k^n
+        /* Compute h_k^n, with h_k and n as in the spec */
+        coset_factor_pow = s->roots_of_unity[pos * FIELD_ELEMENTS_PER_CELL];
+        /* Scale the power of r by h_k^n */
         blst_fr_mul(&weighted_powers_of_r[i], &r_powers[i], &coset_factor_pow);
     }
 

src/eip7594/fft.c

@@ -93,10 +93,14 @@ static void fr_fft_fast(
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The array lengths must be a power of two.
  * @remark Use fr_ifft() for inverse transformation.
  */
 C_KZG_RET fr_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
+
     /* Ensure the length is valid */
     if (n > FIELD_ELEMENTS_PER_EXT_BLOB || !is_power_of_two(n)) {
         return C_KZG_BADARGS;
@@ -116,10 +120,14 @@ C_KZG_RET fr_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The array lengths must be a power of two.
  * @remark Use fr_fft() for forward transformation.
  */
 C_KZG_RET fr_ifft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
+
     /* Ensure the length is valid */
     if (n > FIELD_ELEMENTS_PER_EXT_BLOB || !is_power_of_two(n)) {
         return C_KZG_BADARGS;
@@ -130,7 +138,7 @@ C_KZG_RET fr_ifft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
 
     fr_t inv_len;
     fr_from_uint64(&inv_len, n);
-    blst_fr_inverse(&inv_len, &inv_len);
+    blst_fr_eucl_inverse(&inv_len, &inv_len);
     for (size_t i = 0; i < n; i++) {
         blst_fr_mul(&out[i], &out[i], &inv_len);
     }
@@ -162,19 +170,14 @@ static void g1_fft_fast(
         g1_fft_fast(out, in, stride * 2, roots, roots_stride * 2, half);
         g1_fft_fast(out + half, in + stride, stride * 2, roots, roots_stride * 2, half);
         for (size_t i = 0; i < half; i++) {
-            /* If the point is infinity, we can skip the calculation */
-            if (blst_p1_is_inf(&out[i + half])) {
-                out[i + half] = out[i];
+            /* If the scalar is one, we can skip the multiplication */
+            if (fr_is_one(&roots[i * roots_stride])) {
+                y_times_root = out[i + half];
             } else {
-                /* If the scalar is one, we can skip the multiplication */
-                if (fr_is_one(&roots[i * roots_stride])) {
-                    y_times_root = out[i + half];
-                } else {
-                    g1_mul(&y_times_root, &out[i + half], &roots[i * roots_stride]);
-                }
-                g1_sub(&out[i + half], &out[i], &y_times_root);
-                blst_p1_add_or_double(&out[i], &out[i], &y_times_root);
+                g1_mul(&y_times_root, &out[i + half], &roots[i * roots_stride]);
             }
+            g1_sub(&out[i + half], &out[i], &y_times_root);
+            blst_p1_add_or_double(&out[i], &out[i], &y_times_root);
         }
     } else {
         *out = *in;
@@ -189,10 +192,14 @@ static void g1_fft_fast(
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The array lengths must be a power of two.
  * @remark Use g1_ifft() for inverse transformation.
  */
 C_KZG_RET g1_fft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
+
     /* Ensure the length is valid */
     if (n > FIELD_ELEMENTS_PER_EXT_BLOB || !is_power_of_two(n)) {
         return C_KZG_BADARGS;
@@ -212,10 +219,14 @@ C_KZG_RET g1_fft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The array lengths must be a power of two.
  * @remark Use g1_fft() for forward transformation.
  */
 C_KZG_RET g1_ifft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
+
     /* Ensure the length is valid */
     if (n > FIELD_ELEMENTS_PER_EXT_BLOB || !is_power_of_two(n)) {
         return C_KZG_BADARGS;
@@ -246,14 +257,16 @@ C_KZG_RET g1_ifft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The coset shift factor is RECOVERY_SHIFT_FACTOR.
  */
 C_KZG_RET coset_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
-    C_KZG_RET ret;
-    fr_t *in_shifted = NULL;
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
 
     /* Create some room to shift the polynomial */
-    ret = new_fr_array(&in_shifted, n);
+    fr_t *in_shifted = NULL;
+    C_KZG_RET ret = new_fr_array(&in_shifted, n);
     if (ret != C_KZG_OK) goto out;
 
     /* Shift the poly */
@@ -276,13 +289,15 @@ C_KZG_RET coset_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
  * @param[in]   n   Length of the arrays
  * @param[in]   s   The trusted setup
  *
+ * @remark Will do nothing if given a zero length array.
  * @remark The coset shift factor is RECOVERY_SHIFT_FACTOR. In this function we use its inverse to
  * implement the IFFT.
  */
 C_KZG_RET coset_ifft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
-    C_KZG_RET ret;
+    /* Handle zero length input */
+    if (n == 0) return C_KZG_OK;
 
-    ret = fr_ifft(out, in, n, s);
+    C_KZG_RET ret = fr_ifft(out, in, n, s);
     if (ret != C_KZG_OK) goto out;
 
     shift_poly(out, n, &INV_RECOVERY_SHIFT_FACTOR);

src/eip7594/fk20.c

@@ -56,14 +56,13 @@ static C_KZG_RET toeplitz_coeffs_stride(
  * Compute FK20 cell-proofs for a polynomial.
  *
  * @param[out]  out An array of CELLS_PER_EXT_BLOB proofs
- * @param[in]   p   The polynomial, an array of coefficients
- * @param[in]   n   The length of the polynomial
+ * @param[in]   p   The polynomial, an array of FIELD_ELEMENTS_PER_BLOB coefficients
  * @param[in]   s   The trusted setup
  *
  * @remark The polynomial should have FIELD_ELEMENTS_PER_BLOB coefficients. Only the lower half of
  * the extended polynomial is supplied because the upper half is assumed to be zero.
  */
-C_KZG_RET compute_fk20_proofs(g1_t *out, const fr_t *p, size_t n, const KZGSettings *s) {
+C_KZG_RET compute_fk20_cell_proofs(g1_t *out, const fr_t *p, const KZGSettings *s) {
     C_KZG_RET ret;
     size_t k, k2;
 
@@ -77,7 +76,7 @@ C_KZG_RET compute_fk20_proofs(g1_t *out, const fr_t *p, size_t n, const KZGSetti
     bool precompute = s->wbits != 0;
 
     /* Initialize length variables */
-    k = n / FIELD_ELEMENTS_PER_CELL;
+    k = FIELD_ELEMENTS_PER_BLOB / FIELD_ELEMENTS_PER_CELL;
     k2 = k * 2;
 
     /* Do allocations */
@@ -113,7 +112,9 @@ C_KZG_RET compute_fk20_proofs(g1_t *out, const fr_t *p, size_t n, const KZGSetti
 
     /* Compute toeplitz coefficients and organize by column */
     for (size_t i = 0; i < FIELD_ELEMENTS_PER_CELL; i++) {
-        ret = toeplitz_coeffs_stride(toeplitz_coeffs, p, n, i, FIELD_ELEMENTS_PER_CELL);
+        ret = toeplitz_coeffs_stride(
+            toeplitz_coeffs, p, FIELD_ELEMENTS_PER_BLOB, i, FIELD_ELEMENTS_PER_CELL
+        );
         if (ret != C_KZG_OK) goto out;
         ret = fr_fft(toeplitz_coeffs_fft, toeplitz_coeffs, k2, s);
         if (ret != C_KZG_OK) goto out;

src/eip7594/fk20.h

@@ -29,7 +29,7 @@
 extern "C" {
 #endif
 
-C_KZG_RET compute_fk20_proofs(g1_t *out, const fr_t *p, size_t n, const KZGSettings *s);
+C_KZG_RET compute_fk20_cell_proofs(g1_t *out, const fr_t *p, const KZGSettings *s);
 
 #ifdef __cplusplus
 }

src/eip7594/recovery.c

@@ -281,6 +281,12 @@ C_KZG_RET recover_cells(
      */
     for (size_t i = 0; i < FIELD_ELEMENTS_PER_EXT_BLOB; i++) {
         if (fr_is_null(&cells_brp[i])) {
+            /*
+             * We handle this situation differently because FR_NULL is an invalid value. The right
+             * hand side, vanishing_poly_eval[i], will always be zero when cells_brp[i] is null, so
+             * the multiplication would still be result in zero, but we shouldn't depend on blst
+             * handling invalid values like this.
+             */
             extended_evaluation_times_zero[i] = FR_ZERO;
         } else {
             blst_fr_mul(&extended_evaluation_times_zero[i], &cells_brp[i], &vanishing_poly_eval[i]);