From 518b2ea95f653858a9554f21f9264a10dddb0c2a Mon Sep 17 00:00:00 2001 From: Justin Traglia Date: Mon, 16 Sep 2024 11:17:48 -0500 Subject: [PATCH] Start to fix findings from Dmitry's review --- bindings/java/build.gradle | 4 +- .../java/ethereum/ckzg4844/CKZG4844JNI.java | 12 +++++ bindings/python/README.md | 2 +- src/eip7594/cell.c | 4 +- src/eip7594/eip7594.c | 24 +++++---- src/eip7594/fft.c | 49 ++++++++++++------- src/eip7594/fk20.c | 11 +++-- src/eip7594/fk20.h | 2 +- src/eip7594/recovery.c | 6 +++ 9 files changed, 73 insertions(+), 41 deletions(-) diff --git a/bindings/java/build.gradle b/bindings/java/build.gradle index 2a8dec7ee..04911a01c 100644 --- a/bindings/java/build.gradle +++ b/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() -} \ No newline at end of file +} diff --git a/bindings/java/src/main/java/ethereum/ckzg4844/CKZG4844JNI.java b/bindings/java/src/main/java/ethereum/ckzg4844/CKZG4844JNI.java index 34da7b785..43b18c8f6 100644 --- a/bindings/java/src/main/java/ethereum/ckzg4844/CKZG4844JNI.java +++ b/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; diff --git a/bindings/python/README.md b/bindings/python/README.md index 44fefb05d..f780c7154 100644 --- a/bindings/python/README.md +++ b/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 diff --git a/src/eip7594/cell.c b/src/eip7594/cell.c index 1dccb7df7..39ad3b8de 100644 --- a/src/eip7594/cell.c +++ b/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); } } diff --git a/src/eip7594/eip7594.c b/src/eip7594/eip7594.c index fc4486c5f..3d5909830 100644 --- a/src/eip7594/eip7594.c +++ b/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); } diff --git a/src/eip7594/fft.c b/src/eip7594/fft.c index efc094ed4..ea9e552c5 100644 --- a/src/eip7594/fft.c +++ b/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); diff --git a/src/eip7594/fk20.c b/src/eip7594/fk20.c index 493e7fb70..6f304c48a 100644 --- a/src/eip7594/fk20.c +++ b/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; diff --git a/src/eip7594/fk20.h b/src/eip7594/fk20.h index 76db037ed..b2f9c8e87 100644 --- a/src/eip7594/fk20.h +++ b/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 } diff --git a/src/eip7594/recovery.c b/src/eip7594/recovery.c index f620e50a0..42c0f264a 100644 --- a/src/eip7594/recovery.c +++ b/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]);