refactor(math): dead-code & structural cleanup of the math crate#598
refactor(math): dead-code & structural cleanup of the math crate#598diegokingston wants to merge 19 commits into
Conversation
…r variants
`Deserializable` had a single impl (`Proof<T>`) whose only `deserialize`
call site was inside that impl itself — a recursion with no external
entry point. Production deserialization uses `bincode`/serde everywhere.
Delete the trait and the `Proof<T>` impl.
`merkle_tree/proof.rs` also carried a dead `Serializable` impl behind
`#[cfg(feature = "alloc")]` — a feature the `crypto` crate does not even
define, so the block never compiled. It referenced `math::traits::Serializable`,
which does not exist. Remove it.
Error cleanup (none constructed anywhere in the workspace):
- `DeserializationError` enum + `From<ByteConversionError>` impl — only
reachable through the now-deleted `Deserializable`.
- `PairingError` enum — lambda_vm has no pairings.
- `ByteConversionError::{InvalidValue, PointNotInSubgroup, ValueNotCompressed}`
— the latter two are elliptic-curve concepts; there is no EC here.
- `CreationError::InvalidDecString`.
Pure dead-code removal; `cargo build`/`cargo test -p math` unchanged
(216 tests pass).
`helpers::next_power_of_two(u64)` was bit-for-bit equivalent to stdlib `u64::next_power_of_two()`. Its only caller, `resize_to_next_power_of_two`, was used solely by the `fibonacci_rap` example — production trace sizing already calls stdlib `.next_power_of_two()` directly everywhere. Move `resize_to_next_power_of_two` into `examples/fibonacci_rap.rs` as a local helper (rewritten on `usize` with the stdlib call, dropping the old `try_into().unwrap()` TODO), and delete the `helpers` module.
`evaluate_fft_bit_reversed` was a non-gated `pub fn` but is only called from the FFT property tests — no production caller in stark or prover. Gate it `#[cfg(test)]` so it stops appearing in the production API surface. Restore the gate (or remove it) when a real caller appears.
Both methods are called only from `fast_division` / `invert_polynomial_mod`, which are themselves already `#[cfg(test)]`. No production caller in stark or prover. Gate them `#[cfg(test)]` so they leave the production API surface. `long_division_with_remainder` is left public — it is used by the debug-checks path (`check_boundary_polys_divisibility`) and is a legitimate general polynomial operation other code could want.
The `fft/cpu/` directory implied a `fft/gpu/` sibling that never existed — GPU code lives in the separate `crypto/math-cuda` crate. The `cpu` nesting was a vestigial holdover. Move `bit_reversing`, `bowers_fft`, `roots_of_unity`, `fft`, and `bowers_fft_tests` from `fft/cpu/` up to `fft/`; delete `fft/cpu/mod.rs` and merge its declarations into `fft/mod.rs`. Update every `fft::cpu::...` / `super::cpu::...` path across math, stark, prover, and math-cuda to `fft::...`. Pure module move — no logic change. 216 math tests pass; stark, prover, math-cuda all build.
Two problems compounded: 1. `polynomial/` was a folder holding a single file (`mod.rs`) — the directory level served no purpose. 2. Two files were named `polynomial` (`polynomial/mod.rs` and `fft/polynomial.rs`), both defining `impl Polynomial<...>` blocks. Merge `fft/polynomial.rs` (the FFT-specific `impl Polynomial` blocks plus the `evaluate_fft_cpu` / `interpolate_fft_cpu` free functions) into `polynomial/mod.rs`, then rename it to a top-level `crypto/math/src/polynomial.rs` and delete the folder. Result: one `polynomial.rs` holding the `Polynomial` type and all of its methods (core ring ops + FFT). `fft/` is now purely the FFT *algorithm* layer (Bowers, twiddles, bit-reversal, roots of unity). Also delete the stale `polynomial/README.md` — inherited lambdaworks docs referencing multilinear-polynomial files that don't exist in lambda_vm. Updated the one external path (`fft::polynomial::compose_fft` -> `polynomial::compose_fft`). 216 math + 124 stark tests pass.
Three test conventions coexisted: the `tests/` directory, a sibling `bowers_fft_tests.rs` next to source, and an inline `#[cfg(test)] mod` in the polynomial module. Unify on the `tests/` directory. - Move `fft/bowers_fft_tests.rs` -> `tests/bowers_fft_tests.rs`, register it in `tests/mod.rs`, drop the `mod bowers_fft_tests;` from `fft/mod.rs`. - Move the inline coset-LDE `mod tests` out of `polynomial.rs` into a new `coset_lde_tests` module in `tests/fft_tests.rs`. - Rename `fft/fft.rs` -> `fft/reference_fft.rs`: after the Step 6 flatten it collided with its own parent module name (`fft::fft`), which clippy rejects. The new name also says what it is — the reference radix-2 FFT used only to cross-check the production Bowers FFT. Gate the module `#[cfg(test)] pub(crate)` since its whole content is test-only. 216 math tests pass (same count — no tests lost in the move).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /// Only used by the test-only `fast_division` / `invert_polynomial_mod`. | ||
| #[cfg(test)] | ||
| pub fn truncate(&self, k: usize) -> Self { |
There was a problem hiding this comment.
Medium – Breaking API change
truncate and reverse were previously unconditional pub fns. Gating them with #[cfg(test)] silently removes them from any non-test build. Any downstream crate (or integration test that doesn't run under cfg(test)) that calls poly.truncate(k) will fail to compile.
If the intent is to restrict visibility, use pub(crate) instead of the test gate — that keeps the symbol available in non-test builds while preventing external use:
| pub fn truncate(&self, k: usize) -> Self { | |
| pub(crate) fn truncate(&self, k: usize) -> Self { |
|
|
||
| /// Only used by the test-only `fast_division`. | ||
| #[cfg(test)] | ||
| pub fn reverse(&self, d: usize) -> Self { |
There was a problem hiding this comment.
Same concern as truncate above — #[cfg(test)] removes this from non-test builds, breaking any caller outside a test binary.
| pub fn reverse(&self, d: usize) -> Self { | |
| pub(crate) fn reverse(&self, d: usize) -> Self { |
| /// Currently exercised only by the FFT property tests; gated to keep it out | ||
| /// of the production surface until a non-test caller needs it. | ||
| #[cfg(test)] | ||
| pub fn evaluate_fft_bit_reversed<F: IsFFTField + IsSubFieldOf<E>>( |
There was a problem hiding this comment.
Medium – Breaking API change
evaluate_fft_bit_reversed was previously an unconditional pub fn. Gating it with #[cfg(test)] removes it from non-test builds. The comment says it's "currently exercised only by FFT property tests," but it was part of the public API surface — the FRI commit phase is an obvious future caller. Use pub(crate) to restrict visibility without a test gate.
| E: Send + Sync, | ||
| { | ||
| let scaled = Polynomial::interpolate_fft::<F>(fft_evals)?; | ||
| Ok(scaled.scale(&offset.inv().unwrap())) |
There was a problem hiding this comment.
Low – Pre-existing panic (not introduced here, but now more visible)
offset.inv().unwrap() panics if offset == 0. The function returns Result<_, FFTError> so a zero offset could be turned into a proper error:
| Ok(scaled.scale(&offset.inv().unwrap())) | |
| Ok(scaled.scale(&offset.inv().map_err(|_| FFTError::InvalidInput)?)) |
(adjust to whatever the appropriate FFTError variant is)
Review: Cleanup/math dead codeClean structural refactor — removes ~830 lines of dead code and flattens the Medium — Breaking API changesThree previously-public methods are now gated by
If the goal is to hide these from external crates, Low — Pre-existing panic surfaced by the move
Removed public API (no objection, just noting)The following public items are removed with no replacement:
The removed No issues with the restructuring itselfThe |
Codex Code ReviewNo issues found in the PR diff for security, correctness, significant performance, or simplicity. I attempted to run tests, but the sandbox could not resolve/fetch Cargo dependencies and also hit read-only Cargo/Rustup cache paths. Static review only. |
…tests The reference radix-2 FFT existed only to cross-check the production Bowers FFT. Bowers is already verified directly against a naive O(n^2) DFT at the same sizes, so reference_fft.rs and its four comparison tests are dead weight. gate get_powers_of_primitive_root as test-only (production twiddles go through LayerTwiddles), collapse the evaluate_fft_cpu / interpolate_fft_cpu wrappers into their sole callers, and narrow coset_lde_full_into to pub(crate). Move the inline test modules from bit_reversing.rs, goldilocks.rs and extensions_goldilocks.rs into the dedicated tests/ directory.
Codex Code ReviewFindings
Security No Critical/High/Medium/Low security vulnerabilities found in the diff. Verification I attempted |
Code Review: Cleanup/math dead codeSummary: This PR removes dead code and restructures the Medium — Reduced cross-validation coverage for a security-critical algorithmThe deleted
These cross-checks were the strongest correctness guarantee for the FFT. Removing both the reference implementation and the cross-check tests simultaneously means Bowers FFT is now only validated against the naive O(n²) DFT — a different algorithm family, not an independent implementation of the same algorithm. For a ZK proving system, FFT correctness is directly tied to proof soundness. Consider keeping at least the proptest cross-check by extracting the reference NR FFT into a test-only helper instead of deleting it entirely. Low — Panic on zero offset in public API (
|
| E: Send + Sync, | ||
| { | ||
| let scaled = Polynomial::interpolate_fft::<F>(fft_evals)?; | ||
| Ok(scaled.scale(&offset.inv().unwrap())) |
There was a problem hiding this comment.
Low — panic on zero offset (public API)
offset.inv().unwrap() panics when offset == FieldElement::zero(). A zero coset offset is a degenerate input that should return Err. Consider propagating the error:
| Ok(scaled.scale(&offset.inv().unwrap())) | |
| Ok(scaled.scale(&offset.inv().map_err(|_| FFTError::InvalidOffset)?)) |
Or at minimum document the precondition with a debug_assert!(!offset.is_zero()).
diegokingston
changed the title
…set panic
Addresses PR review feedback.
Reviewers flagged that `#[cfg(test)]`-gating previously-public methods is a
breaking change. Rather than keep them as `pub(crate)` dead code, the cluster
is removed outright — none of it has a production caller:
- Delete `Polynomial::{fast_division, invert_polynomial_mod,
fast_fft_multiplication, truncate, reverse}` and `evaluate_fft_bit_reversed`,
plus their 8 tests. The fast polynomial-division algorithm had no production
use; it existed only to be tested.
- `get_powers_of_primitive_root` stays `#[cfg(test)]` — it is a genuine test
helper used to validate the production FFT, not dead code.
Also fixes the pre-existing panic reviewers noted in `interpolate_offset_fft`:
`offset.inv().unwrap()` panicked on a zero coset offset. Add
`FFTError::InvalidCosetOffset` and propagate it instead.
Continues the dead-code audit of polynomial.rs. Every item removed here has no production caller — each is a function reachable only from its own tests, or a test-file algorithm that tests itself. Removed from polynomial.rs: - to_extension (zero callers anywhere) - long_division_with_remainder, leading_coefficient - ruffini_division_inplace - interpolate_coset_eval_with_g_n_inv (base-field variant) Removed the matching self-referential tests/helpers from polynomial_tests.rs (division_works, division_by_zero_degree_polynomial_works, test_xgcd, ruffini_inplace_* and the div_with_ref / xgcd / ruffini_division helpers). The 3 barycentric tests that exercised the base-field interpolate_coset_eval_with_g_n_inv now call the production-used interpolate_coset_eval_ext_with_g_n_inv (instantiated E=F), so they keep covering the real code path.
1 participant
Summary
Dead-code and structural cleanup of the
mathcrate. Removes unused publicAPI, flattens the FFT module layout, and consolidates tests into one place.
No behavior changes — every removed item has zero production callers.
Changes
Dead code removal
Deserializabletrait and the unreachable error variantsit fed:
DeserializationError,PairingError,ByteConversionError::{InvalidValue, PointNotInSubgroup, ValueNotCompressed},CreationError::InvalidDecString.helpersmodule (next_power_of_two,resize_to_next_power_of_two) — unused.Visibility tightening
Polynomial::{truncate, reverse}andPolynomial::evaluate_fft_bit_reversedbehind
#[cfg(test)]. Every caller is test-only (truncate/reversearereached only through the already-
cfg(test)fast_division/invert_polynomial_mod;evaluate_fft_bit_reversedonly from the FFTproperty tests), so this keeps them out of the production binary.
Structural
fft/cpu/*up tofft/*— thecpulayer had nogpusibling.polynomial/folder into a singlepolynomial.rs; delete thestale
polynomial/README.md.tests/directory.
Folded in (commit 48762cf)
reference_fft.rs(the reference radix-2 FFT) and its fourcomparison tests. It existed only to cross-check the production Bowers FFT,
which is already verified directly against a naive O(n^2) DFT at the same
sizes — the reference oracle was redundant.
get_powers_of_primitive_rootas test-only; production twiddlegeneration goes through
bowers_fft::LayerTwiddles.evaluate_fft_cpu/interpolate_fft_cpuone-line wrappersinto their sole callers; narrow
coset_lde_full_intotopub(crate).bit_reversing.rs,goldilocks.rsand
extensions_goldilocks.rsintotests/.Footprint
25 files in
crypto/math, +674 / -1113. The insertions are mostly thepolynomial/mod.rscontent relocating intopolynomial.rs.Test plan
cargo test -p math— 211 passmake lint(clippy--workspace --all-targets -D warnings) — clean