Skip to content

Tidy up sumcheck RoundCoeffs/RoundProof and add tests#1765

Open
tcoratger wants to merge 1 commit into
binius-zk:mainfrom
tcoratger:thomascoratger/round-coeffs-fieldops-cleanup
Open

Tidy up sumcheck RoundCoeffs/RoundProof and add tests#1765
tcoratger wants to merge 1 commit into
binius-zk:mainfrom
tcoratger:thomascoratger/round-coeffs-fieldops-cleanup

Conversation

@tcoratger

Copy link
Copy Markdown
Contributor

Cleans up the sumcheck round-polynomial helpers and gives the module its first tests.
Pure refactor — no protocol change, no behavior change at any call site.

The problem

RoundCoeffs / RoundProof are the round-polynomial types the sumcheck and MLE-check verifiers use.
Their methods were split across three different trait bounds with no stated reason:

  • evaluate and recover required FieldOps.
  • The endpoint helpers, the Add/Mul/Sum operators required the narrower Field.

The only thing the narrower bound bought was Copy.
So a round polynomial over a packed field could be evaluated but not added or scaled — an arbitrary, undocumented restriction, and a seam inconsistent with the mlecheck twin which already commits to FieldOps.

Two smaller issues rode along:

  • The derived Default demanded $F$: Default even though the type just wraps a Vec, which is always Default.
  • The module had zero tests, while its mlecheck sibling tests the same round-trip.

The idea

Unify every method on FieldOps — the more general bound the abstraction was built for (packed and symbolic fields), and the one evaluate/recover already require.
$F$: Field still satisfies $F$: FieldOps via the blanket impl, so every existing scalar call site is unaffected.

The change

- use binius_field::{Field, field::FieldOps};
+ use binius_field::field::FieldOps;

- impl<F: Field> Add<&Self> for RoundCoeffs<F>      // and Mul, Sum, endpoint helpers
+ impl<F: FieldOps> Add<&Self> for RoundCoeffs<F>
  • Default: replaced the over-constrained derive with a manual impl<F> Default on both types, so they are Default for every $F$ (also required once the bound moved to FieldOps, which does not imply Default).
  • DRY: extracted a private endpoints() -> (F, F) to remove the duplicated (R(0), R(1)) prologue shared by the two endpoint helpers.
  • Cleanup: dropped a redundant where F: FieldOps on recover; added an as_slice accessor to retire the self.0.0 double-tuple in coeffs.
  • Docs: documented the two silent contracts — truncate's non-empty precondition, and why recover subtracts the constant term twice (the two copies cancel in characteristic 2 but keep the identity correct over any field).

Soundness

Nothing changes on the wire.
The bound widening is a pure generalization — every scalar caller resolves to the identical code.
The recover arithmetic is byte-for-byte the same; only its doc and a redundant bound changed.

Scope

  • changed: crates/ip/src/sumcheck/common.rs only.
  • added: a tests module (8 tests) — recover round-trip, the s = R(0) + R(1) identity, both endpoint helpers, ragged-length addition, scaling, iterator Sum, and the empty/zero-polynomial edge case.
  • untouched: every call site; the mlecheck twin (its truncate API-shape divergence is left for a separate change).

Verification

  • cargo check -p binius-ip --all-targets — clean.
  • cargo clippy -p binius-ip — clean.
  • cargo +nightly fmt --all — clean.
  • cargo test -p binius-ip -p binius-ip-prover — 70 passed, 0 failed (8 new).

Note: cargo check --workspace --all-targets currently fails on an unrelated pre-existing error in crates/ip/src/channel.rs (call_native on FieldFn) that reproduces on a clean main under full-workspace feature unification — it is not introduced by this PR.

🤖 Generated with Claude Code

Unify the trait bounds on the round-polynomial helpers around FieldOps.
The endpoint helpers, the arithmetic operators, and the iterator Sum used a
narrower Field bound, while evaluate and recover (and the mlecheck twin)
already required FieldOps. That seam meant a round polynomial over a packed
field could be evaluated but not added or scaled. Everything now lives on
FieldOps, matching what the abstraction was built for, with no caller change.

Fix an over-constrained Default: the derive demanded F: Default even though
the type just wraps a Vec, which is always Default. A manual impl makes both
types Default for every F (also required once the bound moved to FieldOps,
which does not imply Default).

Extract a private endpoints() helper to remove the duplicated (R(0), R(1))
prologue, drop a redundant where-bound on recover, add an as_slice accessor
to retire the self.0.0 double-tuple, and document the two silent contracts
(truncate's non-empty precondition and why recover subtracts the constant
term twice so the identity holds over any field, not only characteristic 2).

Add the module's first tests: recover round-trip, the R(0)+R(1)=s identity,
both endpoint helpers, ragged-length addition, scaling, iterator Sum, and the
empty/zero-polynomial edge case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tcoratger tcoratger requested a review from jimpo as a code owner July 10, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant