test(crypto): cover the Falcon polynomial ring - #3550
Closed
mehmetkr-31 wants to merge 1 commit into
Closed
Conversation
mehmetkr-31
force-pushed
the
test-falcon-polynomial
branch
2 times, most recently
from
August 13, 2026 17:04
b0090df to
da8c3e1
Compare
`math/polynomial.rs` had one test, covering only the negacyclic reduction. Adds 13 more, checking properties against independent references rather than restating the implementation: `galois_adjoint` and `lift_next_cyclotomic` are verified by evaluating at points, multiplication by the evaluation homomorphism, Karatsuba against schoolbook, and `field_norm` against the even/odd split from formula 3.25 of the spec. The alternating sign in `reduce_by_cyclotomic` is pinned directly, since it is the least obvious code in the file. Karatsuba is covered for powers of two only. `vector_karatsuba` splits at `n / 2` and reassembles the high half at offset `n`, so for odd `n` the high half holds one coefficient more than that offset allows and the reassembly indexes past the end; lengths 9 and 17 panic against `next`. Falcon only calls this with `N` = 512, and the reason is recorded next to the test. Part of 0xMiden#3487
mehmetkr-31
force-pushed
the
test-falcon-polynomial
branch
from
August 13, 2026 18:37
da8c3e1 to
75ec8bc
Compare
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #3487 — second slice, covering
math/polynomial.rs. Draft while I wait to be assigned on the issue.field.rsis #3542; the two are independent and can land in either order.Rationale
670 lines, one test — and it covered only the negacyclic reduction. Unlike
field.rs, the doc comments here are already decent, so this slice is tests rather than docs.Two bugs fell out of writing them; neither fix is in this PR.
Not here: the
Polynomial::divzero-dividend fix@Sertug17 found that one first, filed #3534 on 31 July, and has a fix in 0xMiden/crypto#1106. That repo is archived so it cannot land there; they are opening a fresh PR against this one. I had included the fix here before spotting their issue, and removed it at their request — this PR is now purely tests.
Reported, not fixed:
vector_karatsubaon odd lengthsIt splits at
n / 2and reassembles the high half at offsetn. For oddnthe high half holdsceil(n/2)coefficients, one more than that offset accounts for, so the final loop indexes past the end ofproduct. Thezipthat builds the middle term also silently drops the top coefficient of the high half in the same case.Lengths 9 and 17 panic against
nexttoday (index out of bounds: the len is 17 but the index is 17).I deliberately did not fix this. Falcon only ever calls it with
N= 512, so it is latent — butPolynomial::karatsubais public API, and the fix is a choice between handling odd splits correctly and making the power-of-two precondition explicit. That reads like a maintainer decision rather than something to slip into a test PR. The test covers powers of two only, with the reason written next to it. Happy to do either fix.Test plan
The suite checks properties against independent references rather than restating the implementation:
degree/lcis_zerois defined in terms ofreduce_by_cyclotomicX^n = -1,X^(n+1) = -X,X^2n = +1pinned directly — the alternating sign is the least obvious code in the filegalois_adjointf(-x)lift_next_cyclotomicf(x^2)field_normkaratsubaResults
cargo test -p miden-crypto --libpasses (689 tests). Clippy overmiden-crypto --all-targetsis clean under-Dwarnings, andcargo fmt --checkreports nothing.