Fix findings from Dmitry's review

src/eip4844/blob.h

@@ -28,9 +28,6 @@
 /** The number of field elements in a blob. */
 #define FIELD_ELEMENTS_PER_BLOB 4096
 
-/** The number of field elements in an extended blob */
-#define FIELD_ELEMENTS_PER_EXT_BLOB (FIELD_ELEMENTS_PER_BLOB * 2)
-
 /** The number of bytes in a blob. */
 #define BYTES_PER_BLOB (FIELD_ELEMENTS_PER_BLOB * BYTES_PER_FIELD_ELEMENT)
 

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_element = (const Bytes32 *)&cell->bytes[i * BYTES_PER_FIELD_ELEMENT];
- print_bytes32(field_element);
+ const Bytes32 *element_bytes = (const Bytes32 *)&cell->bytes[i * BYTES_PER_FIELD_ELEMENT];
+ print_bytes32(element_bytes);
  }
 }

src/eip7594/cell.h

@@ -30,6 +30,15 @@
 /** The number of bytes in a single cell. */
 #define BYTES_PER_CELL (FIELD_ELEMENTS_PER_CELL * BYTES_PER_FIELD_ELEMENT)
 
+/** The Reed-Solomon erasure coding expansion factor. */
+#define EXPANSION_FACTOR 2
+
+/** The number of field elements in an extended blob */
+#define FIELD_ELEMENTS_PER_EXT_BLOB (FIELD_ELEMENTS_PER_BLOB * EXPANSION_FACTOR)
+
+/** The number of cells in a blob. */
+#define CELLS_PER_BLOB (FIELD_ELEMENTS_PER_BLOB / FIELD_ELEMENTS_PER_CELL)
+
 /** The number of cells in an extended blob. */
 #define CELLS_PER_EXT_BLOB (FIELD_ELEMENTS_PER_EXT_BLOB / FIELD_ELEMENTS_PER_CELL)
 

src/eip7594/eip7594.c

@@ -171,8 +171,8 @@ C_KZG_RET compute_cells_and_kzg_proofs(
  *
  * @param[out] recovered_cells An array of CELLS_PER_EXT_BLOB cells
  * @param[out] recovered_proofs An array of CELLS_PER_EXT_BLOB proofs
- * @param[in] cell_indices The cell indices for the available cells
- * @param[in] cells The available cells we recover from
+ * @param[in] cell_indices The cell indices for the available cells, length `num_cells`
+ * @param[in] cells The available cells we recover from, length `num_cells`
  * @param[in] num_cells The number of available cells provided
  * @param[in] s The trusted setup
  *
@@ -373,13 +373,13 @@ static void deduplicate_commitments(
  * Compute random linear combination challenge scalars for verify_cell_kzg_proof_batch. In this, we
  * must hash EVERYTHING that the prover can control.
  *
- * @param[out] r_powers_out The output challenges
- * @param[in] commitments_bytes The input commitments
+ * @param[out] r_powers_out The output challenges, length `num_cells`
+ * @param[in] commitments_bytes The input commitments, length `num_commitments`
  * @param[in] num_commitments The number of commitments
- * @param[in] commitment_indices The cell commitment indices
- * @param[in] cell_indices The cell indices
- * @param[in] cells The cell
- * @param[in] proofs_bytes The cell proof
+ * @param[in] commitment_indices The cell commitment indices, length `num_cells`
+ * @param[in] cell_indices The cell indices, length `num_cells`
+ * @param[in] cells The cell, length `num_cells`
+ * @param[in] proofs_bytes The cell proof, length `num_cells`
  * @param[in] num_cells The number of cells
  */
 static C_KZG_RET compute_r_powers_for_verify_cell_kzg_proof_batch(
@@ -477,9 +477,9 @@ static C_KZG_RET compute_r_powers_for_verify_cell_kzg_proof_batch(
  * Compute the sum of the commitments weighted by the powers of r.
  *
  * @param[out] sum_of_commitments_out The resulting G1 sum of the commitments
- * @param[in] unique_commitments Array of unique commitments
- * @param[in] commitment_indices Indices mapping to unique commitments
- * @param[in] r_powers Array of powers of r used for weighting
+ * @param[in] unique_commitments Array of unique commitments, length `num_commitments`
+ * @param[in] commitment_indices Indices mapping to unique commitments, length `num_cells`
+ * @param[in] r_powers Array of powers of r used for weighting, length `num_cells`
  * @param[in] num_commitments The number of unique commitments
  * @param[in] num_cells The number of cells
  */
@@ -666,9 +666,12 @@ static C_KZG_RET compute_commitment_to_aggregated_interpolation_poly(
  );
  if (ret != C_KZG_OK) goto out;
 
+ /* Calculate index to the inverse root of unity for this cell index */
+ uint64_t inv_coset_factor_idx = -CELL_INDICES_RBL[i] % FIELD_ELEMENTS_PER_EXT_BLOB;
+ /* For readability, assign root to variable using our index */
+ fr_t *inv_coset_factor = &s->roots_of_unity[inv_coset_factor_idx];
  /* Now divide by the coset shift factor */
- uint64_t pos = -CELL_INDICES_RBL[i] % FIELD_ELEMENTS_PER_EXT_BLOB;
- shift_poly(column_interpolation_poly, FIELD_ELEMENTS_PER_CELL, &s->roots_of_unity[pos]);
+ shift_poly(column_interpolation_poly, FIELD_ELEMENTS_PER_CELL, inv_coset_factor);
 
  /* Update the aggregated poly */
  for (size_t k = 0; k < FIELD_ELEMENTS_PER_CELL; k++) {
@@ -704,11 +707,11 @@ static C_KZG_RET compute_commitment_to_aggregated_interpolation_poly(
  * Compute weighted sum of proofs.
  *
  * @param[out] weighted_proof_lincomb The resulting G1 sum of the proofs scaled by coset factors
- * @param[in] proofs_g1 Array of G1 elements representing the proofs
- * @param[in] r_powers Array of powers of r used for weighting
- * @param[in] cell_indices Array of cell indices
+ * @param[in] proofs_g1 Array of proofs, length `num_cells`
+ * @param[in] r_powers Array of powers of r used for weighting, length `num_cells`
+ * @param[in] cell_indices Array of cell indices, length `num_cells`
  * @param[in] num_cells The number of cells
- * @param[in] s The trusted setup containing roots of unity
+ * @param[in] s The trusted setup
  */
 static C_KZG_RET computed_weighted_sum_of_proofs(
  g1_t *weighted_proof_sum_out,
@@ -719,19 +722,18 @@ static C_KZG_RET computed_weighted_sum_of_proofs(
  const KZGSettings *s
 ) {
  C_KZG_RET ret;
- fr_t coset_factor_pow;
  fr_t *weighted_powers_of_r = NULL;
 
  ret = new_fr_array(&weighted_powers_of_r, num_cells);
  if (ret != C_KZG_OK) goto out;
 
  for (uint64_t i = 0; i < num_cells; i++) {
- /* Compute h_k^n, with h_k and n as in the spec */
- uint64_t pos = CELL_INDICES_RBL[cell_indices[i]];
- coset_factor_pow = s->roots_of_unity[pos * FIELD_ELEMENTS_PER_CELL];
-
+ /* Calculate index to h_k^n; a root to some power is another root */
+ uint64_t h_k_pow_idx = CELL_INDICES_RBL[cell_indices[i]] * FIELD_ELEMENTS_PER_CELL;
+ /* For readability, assign root to variable using our index */
+ fr_t *h_k_pow = &s->roots_of_unity[h_k_pow_idx];
  /* Scale the power of r by h_k^n */
- blst_fr_mul(&weighted_powers_of_r[i], &r_powers[i], &coset_factor_pow);
+ blst_fr_mul(&weighted_powers_of_r[i], &r_powers[i], h_k_pow);
  }
 
  ret = g1_lincomb_fast(weighted_proof_sum_out, proofs_g1, weighted_powers_of_r, num_cells);

src/eip7594/fft.c

@@ -17,7 +17,7 @@
 #include "eip7594/fft.h"
 #include "common/alloc.h"
 #include "common/utils.h"
-#include "eip4844/blob.h"
+#include "eip7594/cell.h"
 #include "eip7594/poly.h"
 
 #include <string.h> /* For memcpy */
@@ -60,10 +60,10 @@ static const fr_t INV_RECOVERY_SHIFT_FACTOR = {
  *
  * Recursively divide and conquer.
  *
- * @param[out] out The results (length `n`)
- * @param[in] in The input data (length `n * stride`)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n * stride`
  * @param[in] stride The input data stride
- * @param[in] roots Roots of unity (length `n * roots_stride`)
+ * @param[in] roots Roots of unity, length `n * roots_stride`
  * @param[in] roots_stride The stride interval among the roots of unity
  * @param[in] n Length of the FFT, must be a power of two
  */
@@ -88,8 +88,8 @@ static void fr_fft_fast(
 /**
  * The entry point for forward FFT over field elements.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *
@@ -115,8 +115,8 @@ C_KZG_RET fr_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
 /**
  * The entry point for inverse FFT over field elements.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *
@@ -136,11 +136,11 @@ C_KZG_RET fr_ifft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
  size_t stride = FIELD_ELEMENTS_PER_EXT_BLOB / n;
  fr_fft_fast(out, in, 1, s->reverse_roots_of_unity, stride, n);
 
- fr_t inv_len;
- fr_from_uint64(&inv_len, n);
- blst_fr_eucl_inverse(&inv_len, &inv_len);
+ fr_t inv_n;
+ fr_from_uint64(&inv_n, n);
+ blst_fr_eucl_inverse(&inv_n, &inv_n);
  for (size_t i = 0; i < n; i++) {
- blst_fr_mul(&out[i], &out[i], &inv_len);
+ blst_fr_mul(&out[i], &out[i], &inv_n);
  }
  return C_KZG_OK;
 }
@@ -154,10 +154,10 @@ C_KZG_RET fr_ifft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
  *
  * Recursively divide and conquer.
  *
- * @param[out] out The results (length `n`)
- * @param[in] in The input data (length `n * stride`)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n * stride`
  * @param[in] stride The input data stride
- * @param[in] roots Roots of unity (length `n * roots_stride`)
+ * @param[in] roots Roots of unity, length `n * roots_stride`
  * @param[in] roots_stride The stride interval among the roots of unity
  * @param[in] n Length of the FFT, must be a power of two
  */
@@ -187,8 +187,8 @@ static void g1_fft_fast(
 /**
  * The entry point for forward FFT over G1 points.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *
@@ -214,8 +214,8 @@ C_KZG_RET g1_fft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
 /**
  * The entry point for inverse FFT over G1 points.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *
@@ -235,11 +235,11 @@ C_KZG_RET g1_ifft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
  size_t stride = FIELD_ELEMENTS_PER_EXT_BLOB / n;
  g1_fft_fast(out, in, 1, s->reverse_roots_of_unity, stride, n);
 
- fr_t inv_len;
- fr_from_uint64(&inv_len, n);
- blst_fr_eucl_inverse(&inv_len, &inv_len);
+ fr_t inv_n;
+ fr_from_uint64(&inv_n, n);
+ blst_fr_eucl_inverse(&inv_n, &inv_n);
  for (size_t i = 0; i < n; i++) {
- g1_mul(&out[i], &out[i], &inv_len);
+ g1_mul(&out[i], &out[i], &inv_n);
  }
 
  return C_KZG_OK;
@@ -252,8 +252,8 @@ C_KZG_RET g1_ifft(g1_t *out, const g1_t *in, size_t n, const KZGSettings *s) {
 /**
  * Do an FFT over a coset of the roots of unity.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *
@@ -284,8 +284,8 @@ C_KZG_RET coset_fft(fr_t *out, const fr_t *in, size_t n, const KZGSettings *s) {
 /**
  * Do an inverse FFT over a coset of the roots of unity.
  *
- * @param[out] out The results (array of length n)
- * @param[in] in The input data (array of length n)
+ * @param[out] out The results, length `n`
+ * @param[in] in The input data, length `n`
  * @param[in] n Length of the arrays
  * @param[in] s The trusted setup
  *

src/eip7594/fk20.c

@@ -25,31 +25,27 @@
 /**
  * Reorder and extend polynomial coefficients for the toeplitz method, strided version.
  *
- * @param[out] out The reordered polynomial, size `n * 2 / stride`
- * @param[in] in The input polynomial, size `n`
- * @param[in] n The size of the input polynomial
+ * @param[out] out The reordered polynomial, length `CELLS_PER_EXT_BLOB`
+ * @param[in] in The input polynomial, length `FIELD_ELEMENTS_PER_BLOB`
  * @param[in] offset The offset
- * @param[in] stride The stride
  */
-static C_KZG_RET toeplitz_coeffs_stride(
- fr_t *out, const fr_t *in, size_t n, size_t offset, size_t stride
-) {
- size_t k, k2;
-
- if (stride == 0) return C_KZG_BADARGS;
+static void toeplitz_coeffs_stride(fr_t *out, const fr_t *in, size_t offset) {
+ /* Calculate starting indices */
+ size_t out_start = CELLS_PER_BLOB + 2;
+ size_t in_start = CELLS_PER_EXT_BLOB - offset - 1;
 
- k = n / stride;
- k2 = k * 2;
+ /* Set the first element */
+ out[0] = in[FIELD_ELEMENTS_PER_BLOB - 1 - offset];
 
- out[0] = in[n - 1 - offset];
- for (size_t i = 1; i <= k + 1 && i < k2; i++) {
+ /* Initialize these elements to zero */
+ for (size_t i = 1; i < out_start; i++) {
  out[i] = FR_ZERO;
  }
- for (size_t i = k + 2, j = 2 * stride - offset - 1; i < k2; i++, j += stride) {
- out[i] = in[j];
- }
 
- return C_KZG_OK;
+ /* Copy elements with a fixed stride */
+ for (size_t i = 0; i < CELLS_PER_EXT_BLOB - out_start; i++) {
+ out[out_start + i] = in[in_start + i * FIELD_ELEMENTS_PER_CELL];
+ }
 }
 
 /**
@@ -77,7 +73,7 @@ C_KZG_RET compute_fk20_cell_proofs(g1_t *out, const fr_t *p, const KZGSettings *
 
  /* Initialize length variables */
  k = FIELD_ELEMENTS_PER_BLOB / FIELD_ELEMENTS_PER_CELL;
- k2 = k * 2;
+ k2 = k * EXPANSION_FACTOR;
 
  /* Do allocations */
  ret = new_fr_array(&toeplitz_coeffs, k2);
@@ -112,10 +108,7 @@ C_KZG_RET compute_fk20_cell_proofs(g1_t *out, const fr_t *p, const KZGSettings *
 
  /* 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, FIELD_ELEMENTS_PER_BLOB, i, FIELD_ELEMENTS_PER_CELL
- );
- if (ret != C_KZG_OK) goto out;
+ toeplitz_coeffs_stride(toeplitz_coeffs, p, i);
  ret = fr_fft(toeplitz_coeffs_fft, toeplitz_coeffs, k2, s);
  if (ret != C_KZG_OK) goto out;
  for (size_t j = 0; j < k2; j++) {

src/eip7594/poly.c

@@ -31,7 +31,7 @@
  * Multiplies each coefficient by `shift_factor ^ i`. Equivalent to creating a polynomial that
  * evaluates at `x * shift_factor` rather than `x`.
  *
- * @param[in,out] p The polynomial coefficients to be scaled
+ * @param[in,out] p The polynomial coefficients to be scaled, length `p`
  * @param[in] len Length of the polynomial coefficients
  * @param[in] shift_factor Shift factor
  */