Fix edge-case bugs in polynomial arithmetic, the specialized FFT, and Merkle path parsing by ebfull · Pull Request #269 · zakura-core/common

ebfull · 2026-08-29T21:09:28Z

Six small robustness fixes from a review of edge-case handling, one commit each:

EvaluationDomain::l_i_range (zakura-halo2-proofs) evaluated every requested Lagrange basis polynomial to zero whenever the evaluation point was itself a domain point: the zero denominator survives batch inversion while the shared numerator xn - 1 also vanishes, so the removable 0/0 singularity collapsed to zero instead of the documented l_i(omega^i) = 1. The surviving zero now resolves to one; non-domain points take the unchanged path.
Polynomial addition (zakura-halo2-proofs) zipped each parallel chunk of the left operand with a suffix of the right, so mismatched-length operands were truncated, kept an unsummed tail, or panicked out of bounds depending on operand order and thread count. It now asserts equal lengths. The multiopen prover's collapse test reference is rewritten to state its intended ragged-group semantics directly instead of leaning on the old truncating operator.
small_multiexp (zakura-halo2-proofs) silently ignored extra bases and read out of bounds for a missing one; it now asserts equal input lengths, matching best_multiexp.
CurveExt::fft_vartime (zakura-pasta-curves) always reported success, but its fused 8- and 16-point codelets are only correct when omega has exact multiplicative order 2^log_n. It now returns false without touching the output for any other root, and the in-tree caller already falls back to the generic FFT on that signal.
CurveExt::hash_to_curve (zakura-pasta-curves) returns a closure that panics on over-long domain prefixes with no documented bound. The exact limit (227 bytes for Pallas, 228 for Vesta, checked on the first message rather than at construction) is now a documented # Panics contract with boundary tests on both sides.
merkle_path_from_slice (zakura-primitives) indexed the depth byte before checking that the input was non-empty, so an empty slice panicked instead of returning an io::Error like every other malformed encoding.

Every fix carries regression tests. Full --release test suites pass for all three crates, cargo fmt --check is clean, and the changelog fragment validates with ./scripts/changelog.py check.

🤖 Generated with Claude Code

merkle_path_from_slice indexed the depth byte before checking that the input was non-empty, so an empty serialized Merkle path panicked with an index-out-of-bounds instead of returning an io::Error like every other malformed encoding. Read the depth byte fallibly with split_first, and add a regression test asserting that every strict prefix of a valid encoding (including the empty slice) returns Err instead of panicking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

small_multiexp silently ignored extra bases, and a missing base panicked with an out-of-bounds index only when the unpaired scalar had a bit set. Assert up front that coeffs and bases have equal lengths, matching the contract best_multiexp already documents and enforces, and add regression tests for both mismatch directions. Mismatched inputs that previously returned a value now panic, but the old behavior was undocumented and inconsistent with the sibling MSM, and no caller is known to pass mismatched lengths (the sole in-tree caller is a benchmark). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The Pasta implementation of CurveExt::fft_vartime always reported success, but its fused 8- and 16-point codelets use factorization identities that hold only when omega has exact multiplicative order 2^log_n. For an accepted lower-order root such as omega = 1 it returned true with a transform that differs from the generic radix-2 path, so enabling the GLV specialization could silently change results. Check the order up front (omega^(2^(log_n - 1)) == -1, or omega == 1 for log_n = 0) and return false without modifying the output, which the trait already defines as the decline signal and the in-tree halo2 caller already handles by falling back. Document the decline case on the trait method and add regression tests at the codelet thresholds covering omega = 1, a lower-order root, and a higher-order root. Exact-order roots see identical behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The Add impl for Polynomial zipped each parallel chunk of the left operand with a suffix of the right operand, so mismatched-length operands were silently truncated, kept an unsummed tail, or panicked out of bounds depending on operand order and Rayon thread count. Assert equal lengths before parallelizing, document the panic, and add regression tests for a same-length sum and both mismatch directions. The multiopen prover's streaming-collapse test compared against a reference built on the old truncating operator; rewrite the reference to state the intended semantics directly: group members are zero-extended or truncated to the group head's length, matching fold_polynomial_range's treatment of absent coefficients. Production callers are unaffected, since in-tree additions always combine polynomials sized to the same domain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

l_i_range documents that l_i(omega^i) = 1, but evaluated every requested basis polynomial to zero whenever x was a domain point: the denominator x - omega^i vanishes at the matching rotation and batch_invert leaves zeros in place, while the shared numerator xn - 1 vanishes too, so the removable 0/0 singularity collapsed to zero. The PLONK verifier could thus report l_0, l_last, or l_blind as zero for a subgroup challenge — a negligible-probability event under an honest Fiat-Shamir transcript, but a divergence from the specified identity. A zero surviving batch inversion occurs exactly when x is that rotation's own domain point, so substitute the cancelled value one there; every other rotation keeps its correct zero from the vanishing numerator, and non-domain points take the unchanged path. Add a regression test sweeping every domain point against all rotations in -7..=7, plus a domain point whose own rotation is not requested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

CurveExt::hash_to_curve accepts an arbitrary &str domain prefix, but the returned closure asserts that the encoded domain-separation tag, domain_prefix || "-" || CURVE_ID || "_XMD:BLAKE2b_SSWU_RO_", fits the one-byte length required by the hash-to-curve construction, panicking on the first message for longer prefixes. The trait documented no length restriction, so callers had no way to reject oversized prefixes without duplicating an internal detail. The 255-byte tag limit is a stable property of the DST encoding and the failure is loud and deterministic, so document the exact bound as a # Panics section — domain_prefix may be at most 233 - CURVE_ID.len() bytes (227 for Pallas, 228 for Vesta), checked on the first message rather than at construction — and add boundary tests for both curves at the longest accepted prefix and the shortest rejected one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ebfull · 2026-08-29T21:10:41Z

@v12sec pls review

v12-auditor · 2026-08-29T21:10:48Z

Note

Complete: Audit complete. V12 found one issue worth reviewing.

Open the full results here.

Finding	Severity	Details
F-258590	🟡 `Medium`	Short polynomials panic multiopen proving The public `multiopen::create_proof` API accepts `ProverQuery` values containing arbitrary coefficient polynomials but validates only empty and duplicate query sets. `collapse_polynomials_with` preserves the first polynomial's length, while quotient construction resizes `q_prime_poly` to `params.n`. The final fold then adds each unchanged-length collapsed polynomial to that `params.n`-length accumulator. The diff's new equal-length assertion in `Polynomial::add` therefore panics whenever a point-set group's first polynomial is not exactly `params.n` coefficients. A concrete reproduction uses size-two parameters, a one-coefficient polynomial from a public size-one `EvaluationDomain`, and a single query: collapse returns length one, Kate division followed by resize produces a length-two quotient, and the final addition reaches `assert_eq!(2, 1)`. This short polynomial has an unambiguous zero-extended representation, matching the ragged semantics retained by the multiopen collapse logic, so an unrecoverable panic is not necessary.

Analyzed 10 files, diff 629e89f...65c2708.

The equal-length assertion added to polynomial addition made the public multiopen::create_proof panic in its final fold whenever a point-set group's first polynomial had fewer coefficients than the parameters: collapsing preserves that length, while the quotient accumulator is always resized to the parameters' length. Such queries previously produced valid proofs, since a short coefficient polynomial has an unambiguous zero-extended reading — the same one the collapse logic already gives shorter group members and a commitment gives absent high coefficients. Zero-extend each collapsed polynomial to the parameters' length before folding, and return the existing InvalidInput error for a query polynomial with more coefficients than the parameters support, which can never be committed under them and was previously truncated silently. Proofs for domain-sized polynomials are unchanged. Add a prove-and-verify regression test mixing a one-coefficient query with a domain-sized one, plus an oversized-query error test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ValarDragon · 2026-08-29T23:25:08Z

+    // Each collapsed polynomial keeps its group head's length. Zero-extend to
+    // the parameters' length — absent high coefficients read as zero, exactly
+    // as the polynomial's commitment treats them — so the final combination
+    // below folds equal-length operands.
+    for q_poly in &mut q_polys {
+        q_poly.values.resize(params.n as usize, C::Scalar::ZERO);
+    }


Need to check if this affects performance, this seems bad for efficiency

ValarDragon · 2026-08-29T23:26:22Z

+            *result = if result.is_zero_vartime() {
+                // The denominator `x - omega^i` was zero and survived batch
+                // inversion as zero: `x` is this basis polynomial's own
+                // domain point, so the shared numerator `xn - 1` vanishes
+                // too and the removable singularity cancels to
+                // `l_i(omega^i) = 1`.
+                F::ONE
+            } else {
+                self.rotate_omega(*result * common, rotation)
+            };


Also need to check if this is a performance sensitive routine.

ValarDragon · 2026-08-29T23:26:30Z

+///
+/// This function will panic if coeffs and bases have a different length.
 pub fn small_multiexp<C: CurveAffine>(coeffs: &[C::Scalar], bases: &[C]) -> C::Curve {
+    assert_eq!(coeffs.len(), bases.len());


ValarDragon · 2026-08-29T23:26:44Z

+    /// This function will panic if the operands have different lengths,
+    /// i.e. were created for different domain sizes.
    fn add(mut self, rhs: &'a Polynomial<F, B>) -> Polynomial<F, B> {
+        assert_eq!(self.values.len(), rhs.values.len());


ValarDragon · 2026-08-29T23:28:47Z

+    // `omega` has exact order `2^log_n` iff `omega^(2^(log_n - 1)) == -1`
+    // (for `log_n == 0`, iff `omega == 1`).
+    let exact_order = if log_n == 0 {
+        omega == C::ScalarExt::ONE
+    } else {
+        let mut half_order_power = omega;
+        for _ in 0..log_n - 1 {
+            half_order_power = half_order_power.square();
+        }
+        half_order_power == -C::ScalarExt::ONE
+    };
+    if !exact_order {
+        return false;
+    }


Is this really needed? Can't we just write this as an invariant the caller must fulfill else the behavior is undefined?

ebfull and others added 7 commits August 29, 2026 15:08

Add changelog fragment for PR #269

65c2708

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ValarDragon reviewed Aug 29, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix edge-case bugs in polynomial arithmetic, the specialized FFT, and Merkle path parsing - #269

Fix edge-case bugs in polynomial arithmetic, the specialized FFT, and Merkle path parsing#269
ebfull wants to merge 8 commits into
mainfrom
edge-case-fixes

ebfull commented Aug 29, 2026

Uh oh!

ebfull commented Aug 29, 2026

Uh oh!

v12-auditor Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

ValarDragon Aug 29, 2026

Uh oh!

ValarDragon Aug 29, 2026

Uh oh!

ValarDragon Aug 29, 2026

Uh oh!

ValarDragon Aug 29, 2026

Uh oh!

ValarDragon Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ebfull commented Aug 29, 2026

Uh oh!

ebfull commented Aug 29, 2026

Uh oh!

v12-auditor Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValarDragon Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

ValarDragon Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

ValarDragon Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

ValarDragon Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

ValarDragon Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

v12-auditor Bot commented Aug 29, 2026 •

edited

Loading