harden the deep-composition fuse: pin its two panic guards, make them local#834
Open
MauroToscano wants to merge 7 commits into
Open
harden the deep-composition fuse: pin its two panic guards, make them local#834MauroToscano wants to merge 7 commits into
MauroToscano wants to merge 7 commits into
Conversation
…FRI points reconstruct_deep_composition_poly_evaluation walked the OOD table and trace-term coefficients, and inverted denominators, independently for the regular and symmetric evaluation points, then recomputed the point-invariant OOD/gamma sums from scratch on every one of the ~80 FRI queries per proof. Rewriting coeff*(base-ood)*denom as denom*(coeff*base - coeff*ood) isolates the point-independent coeff*ood term (identical between the two points) from coeff*base, so both points can share the OOD walk and a single batch-inverse. The point-invariant ood_row_sum/z_pow/h_sum_zpow sums are now computed once per proof in compute_query_invariant_deep_terms instead of once per query. Multi-query recursion profile: 1,325,335,927 -> 916,824,543 cycles (-30.8%); step 3 (FRI) 635,418,644 -> 233,869,693 cycles (-63.2%).
…uards
The fused deep-composition reconstruction made two guards in
reconstruct_deep_composition_poly_evaluation_pair the sole protection
against a release-mode out-of-bounds panic on a malformed proof, and both
run in step 3 — before step 4's Merkle openings could reject the tampering:
- num_base != num_base_sym: the base-column branch is bounded by the
regular num_base but resolves symmetric columns via base_at_sym, which
indexes the symmetric slices.
- the composition-part length check: the part loop is bounded by the
proof-level number_of_parts and indexes both per-query slices with it.
Neither was covered. Both tests use the LogUp read-only-memory RAP, which
has auxiliary columns (trace_layout = (5, 1)) and two composition parts, so
they reach the base/aux split and the multi-part loop rather than the
degenerate one-part case.
Verified by removing each guard in turn: both tests then fail with an index
out of bounds, not merely a false verdict.
The second guard's symmetric arm reused the regular num_base, so it was only correct because the num_base != num_base_sym guard above had already run. Check num_base_sym instead, making the two guards independently correct rather than order-dependent. No behavior change: the guard above forces num_base == num_base_sym before this arm is reached, so the two forms agree on every input that gets here.
…roduct compute_query_invariant_deep_terms and reconstruct_deep_composition_poly_evaluation_pair direct-index trace_term_coeffs[col][row] for col < ood_width, row < ood_height. The product guard (len * [0].len() != height * width) accepts shapes that transpose those bounds, and only inspected column 0's length; it was sufficient only via a three-link chain across files: #815's OOD-table guard in multi_verify_views pins the width, replay_rounds_after_round_1 builds trace_term_coeffs by exact division, and every AIR happens to satisfy the unasserted trace_columns == trace_layout.0 + trace_layout.1 convention. Check the shape the code actually needs instead, so the two functions are locally panic-free. The is_empty() check goes away because the new form never indexes [0]: an empty trace_term_coeffs is rejected by the length compare whenever width > 0, and the degenerate width == 0 case indexes nothing (#815 already rejects height == 0). Runs once per proof, integer compares only — no per-query cost, no field operations.
… comments base_row_sum/base_row_sum_sym accumulate base AND aux columns, while num_base/base_at in the same function mean "base-field, not aux". The name asserted a partition the code does not implement, right at the identity under review. Pure rename, no codegen impact; the fn doc-comment's coeff*base becomes coeff*lde to match. The comment above primitive_root still described the pre-fuse code: the per-element base_at(col) - ood_val subtraction is now base_at(col) * coeff, a Mul. F->E subtractions via IsSubFieldOf do still exist in the helper (the evaluation_point - current_z denominators), so the reworded comment only claims the Mul for the trace-term path.
…ices reconstruct_deep_composition_poly_evaluation_pair took 14 arguments, eight of which the caller derived from a single Copy DeepPolynomialOpeningView. Pass the view and resolve the slices in the callee, and move ood_width into QueryInvariantDeepTerms so the proof param drops too: 14 args -> 6, and #[allow(clippy::too_many_arguments)] is deleted rather than inherited. The inner row/column accumulation is textually unchanged. Slice resolution happens once per query either way, only on the other side of the call, and the per-query proof.trace_ood_evaluations().width() call is now a struct read. This commit is cosmetic and can be dropped independently if the signature change is unwelcome in the hot loop.
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.
Stacks onto #826 (
perf/deep-composition-fuse-hoist). Hardening and polish only — the algebra of the fuse/hoist is untouched. It was independently verified as exact, and this PR does not change it.Tests: two guards that had no coverage
The fuse made two guards in
reconstruct_deep_composition_poly_evaluation_pairthe sole protection against a release-mode out-of-bounds panic on a malformed proof. Both run in step 3, before step 4's Merkle openings could reject the tampering, so nothing else stands between a malicious proof and the panic:num_base != num_base_sym— the base-column branch is bounded by the regularnum_basebut resolves symmetric columns throughbase_at_sym, which indexes the symmetric slices.number_of_partsand indexes both the regular and symmetric per-query slices with it.Both tests use the LogUp read-only-memory RAP (
trace_layout() == (5, 1),composition_poly_degree_bound() == 2 * trace_length) so they exercise the base/aux split and the multi-part loop rather than the degenerate one-part case.Each test was verified by removing the guard it pins and confirming it fails with an index out of bounds — not merely a
falseverdict:num_base != num_base_symbase_at_sym:len is 4 but the index is 4..._sym[j]:len is 1 but the index is 1The second panic's
len 1(popped from 2) confirms the multi-part loop is genuinely covered.Note on the first test: it moves one column from the symmetric main trace into the symmetric aux trace rather than simply popping. A plain pop is caught by the width guard once that guard is fixed (below), so it would no longer pin the base/aux split guard. Preserving the total width isolates the guard under test.
num_base_symin the width guardThe second guard's symmetric arm reused the regular
num_base, so it was correct only because the guard above it had already run. It now checksnum_base_sym, making the two guards independently correct instead of order-dependent.No behavior change: the guard above forces
num_base == num_base_symbefore this arm is reached, so the two forms agree on every input that gets here. This is unobservable by design and therefore has no test of its own — a test would have to remove the other guard to see it.Shape guard instead of a dimension product
compute_query_invariant_deep_termsandreconstruct_deep_composition_poly_evaluation_pairdirect-indextrace_term_coeffs[col][row]forcol < ood_width,row < ood_height. The product guard (len * [0].len() != height * width) accepts shapes that transpose those bounds and only inspected column 0's length. It was sufficient only through a three-link chain across files: #815's OOD-table guard inmulti_verify_viewspins the width,replay_rounds_after_round_1builds the coefficients by exact division, and every AIR happens to satisfy the unassertedtrace_columns == trace_layout.0 + trace_layout.1convention. Nothing was exploitable — this makes panic-freedom local rather than dependent on a guard in another function.The new check is integer compares, once per proof — no per-query cost, no field operations. Dropping
is_empty()is safe because the new form never indexes[0]: an emptytrace_term_coeffsis rejected by the length compare wheneverwidth > 0, and the degeneratewidth == 0case indexes nothing (#815 already rejectsheight == 0).Nits
base_row_sum/base_row_sum_sym→lde_row_sum/lde_row_sum_sym: they accumulate base and aux columns, whilenum_base/base_atin the same function mean "base-field, not aux". The name asserted a partition the code does not implement, right at the identity under review. Pure rename.primitive_rootstill described the pre-fuse code: the per-elementbase_at(col) - ood_valsubtraction is nowbase_at(col) * coeff, a Mul. Reworded for the trace-term path only —F→Esubtractions viaIsSubFieldOfdo still exist in the helper (theevaluation_point - current_zdenominators).Last commit is droppable
refactor(verifier): pass the opening view instead of eight derived slicesreduces the signature from 14 args to 6 and deletes#[allow(clippy::too_many_arguments)]rather than inheriting it. The inner row/column accumulation is textually unchanged; slice resolution happens once per query either way, just on the other side of the call, and the per-queryproof.trace_ood_evaluations().width()call becomes a struct read. Drop this commit if the signature change is unwelcome in the hot loop.Caveat: I could not measure the in-VM recursion cycle count locally, so "no regression" for that commit rests on the inner loop being textually identical and the moved work being per-query, not per-element — not on a profile.
Checks
cargo test --release -p stark --lib— 187 passed (185 pre-existing + 2 new).cargo clippy -p stark --all-targetsclean (the 19 remaining warnings are pre-existing, inprover_tests.rs/fri_tests.rs/row_pair_opening_tests.rs).cargo fmt --checkclean.