Skip to content

fix: unbreak CI (clippy lint, and the linker on the test job) - #459

Open
gopikannappan wants to merge 2 commits into
Plonky3:mainfrom
gopikannappan:fix-ci
Open

gopikannappan wants to merge 2 commits into
Plonky3:mainfrom
gopikannappan:fix-ci

Conversation

@gopikannappan

Copy link
Copy Markdown

main's CI is red on two jobs, so every pull request opened against it inherits the failure. #458 has carried a red "Formatting and Clippy" since 10 September for this reason rather than anything in its own diff.

The two failures have nothing to do with each other.

Clippy

cargo clippy --all-targets -- -D warnings rejects twenty uses of chunks_exact at a constant size, across circuit, circuit-prover and their tests. A recent stable promoted that lint; nothing in the repository changed.

as_chunks is what it asks for, and it reads better where the pair is destructured:

// before
for (j, pair) in outputs.chunks_exact(2).enumerate() {
    row[a + j] = pair[0];
    row[b + j] = pair[1];
}

// after
let (pairs, _) = outputs.as_chunks::<2>();
for (j, &[low, high]) in pairs.iter().enumerate() {
    row[a + j] = low;
    row[b + j] = high;
}

The remainder is dropped exactly as chunks_exact dropped it, so behaviour is unchanged at every call. Plonky3 took the same route for the same lint last week.

The workspace test job

This one is not a failing test. The linker dies:

error: linking with `cc` failed: exit status: 1
collect2: fatal error: ld terminated with signal 7 [Bus error], core dumped

Signal 7 out of ld on a hosted runner is the shape of exhausted memory, and cargo nextest run --workspace --all-targets asks it to link every test binary in the workspace with full debug info.

The second commit drops debug info in CI, beside the CARGO_INCREMENTAL: 0 you already carry for the neighbouring reason. It also makes the AVX2 leg append to RUSTFLAGS instead of replacing it, since replacing would have dropped the new flag on the one leg that also fails.

I want to be straight about what I can and cannot show here. I cannot reproduce the crash, so I cannot prove that fixes it. What I can say is that the same workspace links and runs clean on aarch64-apple-darwin: 996 tests across 20 binaries, exit 0, on 8f9876e with these changes. If you would rather solve the runner pressure another way, drop that commit and take the clippy one; it is the half that unblocks #458's red check either way.

Checked

  • cargo clippy --all-targets -- -D warnings: clean.
  • cargo +nightly fmt --all -- --check: clean.
  • cargo test --workspace: 996 passed, 0 failed.
  • p3-circuit lib and its blake3_compress test after the edits: 418 and 7 passing.

🤖 Generated with Claude Code

gopikannappan and others added 2 commits September 29, 2026 12:56
`cargo clippy --all-targets -- -D warnings` fails on main with twenty
`clippy::manual_slice_size_calculation`-style rejections of
`chunks_exact` at a constant size, so the lint job has been red for
everyone and any new pull request inherits the failure.

`as_chunks` gives the same slices with their length in the type, which
is what the lint asks for: the loop body destructures `[low, high]`
rather than indexing a slice the compiler cannot bound.

The remainder is dropped exactly as `chunks_exact` dropped it, so every
call keeps its behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workspace test job does not fail a test. Its linker dies:

    error: linking with `cc` failed: exit status: 1
    collect2: fatal error: ld terminated with signal 7 [Bus error]

Signal 7 out of `ld` on a hosted runner is the shape of exhausted
memory, and `--all-targets` asks it to link every test binary in the
workspace with full debug info. The env already carries
`CARGO_INCREMENTAL: 0` for the neighbouring reason, so this sits beside
it: nothing in CI reads a backtrace, and dropping debug info is the
cheapest thing that makes those binaries smaller.

The AVX2 leg used to replace `RUSTFLAGS` rather than extend it, which
would have dropped the new flag on the one leg that also fails, so it
now appends.

I cannot reproduce the crash to prove this fixes it: the same workspace
links and runs clean on aarch64-darwin, 996 tests across 20 binaries.
Drop this commit if you would rather solve it another way.

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

Copy link
Copy Markdown
Author

CI on this branch says the diagnosis was right and turns up one thing underneath it.

Clippy passes, so the lint half is settled and #458's red check clears with it.

The linker survives. Build and Test (ubuntu-latest) got all the way through linking and ran the suite: 1304 tests, 1208 s. Before this branch it died at ld with signal 7 before running anything. So dropping debug info does fix the crash, which I could not show locally.

That reveals exactly one failing test, which the crash had been hiding: test_batch_verifier_hiding_mmcs, 1303 of 1304 passed.

It is not environmental. It fails the same way on aarch64-apple-darwin as on your Linux runner:

thread 'test_batch_verifier_hiding_mmcs' panicked at p3-lookup-0.8.0/src/debug_util.rs:82:
  Lookup mismatch ...
thread 'test_batch_verifier_hiding_mmcs' panicked at recursion/tests/zk_hiding_mmcs.rs:568:
  assertion failed: matches!(&local_result,
    Err(ProofCheckError::DebugPanic(rejection_oracle::DebugRejectionKind::Constraint)))

The mutation is still rejected, which is the property that matters. It is rejected as DebugRejectionKind::Lookup rather than Constraint: mutate_salt_leaf_poseidon_trace now trips the lookup argument before any constraint fails, and the assertion pins the kind.

It dates from 12e86cd, the 0.8.0 upgrade. That commit also moved the randomization parameter from 2 to 4 in this test, which looked like the obvious culprit, so I tried putting it back: the test then fails earlier still, at prove_batch on line 271. So 4 is required by the new API and reverting is not the answer.

I have not touched the assertion, because the two readings need someone who knows the intent:

  • if the salt-leaf mutation is simply caught earlier now, the assertion wants widening to accept either kind;
  • if a constraint that used to catch it no longer does, widening would paper over a real regression in the circuit.

I cannot tell which from outside, and this is a test about a mutation being detected, which is the wrong place to guess. Happy to push either change once you say which it is, or to leave it to you entirely.

To be explicit about what that means for this PR: Build and Test stays red here, on a failure that is already on main and was previously masked. The two commits still take CI from "fails before running anything" to "runs everything and reports one real failure", which seemed worth having either way.

@gopikannappan

Copy link
Copy Markdown
Author

I said earlier I could not tell which of the two readings applied. I can now, and it is the one that argues against the obvious fix.

The two mutations in this test are deliberately different. Their messages say so:

  • mutate_salt_leaf_poseidon_trace — "a changed salt coefficient in the physical leaf-hash row" — asserts Constraint;
  • mutate_salt_alu_bus_row — "a changed salt witness disconnected from the honest physical leaf-hash row" — asserts Lookup.

So the test is not merely checking that tampering is rejected. It is pinning which layer rejects it: the first case says the physical leaf-hash row is constrained locally, the second says a disconnected witness is caught by the global lookup.

The first mutation can no longer be caught locally, by construction. It edits Poseidon2Trace::operations[..].input_values, and the prover builds the AIR rows from those operations with air.generate_trace_rows(&ops, &constants, 0). The permutation is re-run over the mutated input, so the row that reaches the AIR is internally consistent: input, output and every intermediate agree. No Poseidon2 constraint can object. Only the bus notices, because the circuit still expects the honest tuple.

I confirmed the second half of that: changing the expectation to Lookup makes the test pass, 7 of 7.

Which is why I do not think flipping that word is the right fix. It would turn the first case into a second copy of the one below it, and quietly retire the only check that the physical leaf-hash row is locally constrained. The test would go green while testing less than it was written to test.

Two honest options, and the choice is yours:

  • Preserve the intent: mutate the generated trace matrix rather than the operation inputs, so the row is inconsistent and the local constraint has something to catch. This keeps the distinction the test was built around.
  • Retire it deliberately: if local detection at this layer is no longer meaningful after the 0.8.0 prover changes, say so and reduce the case to a plain "is rejected" assertion, with a comment recording why.

Either way it is a deliberate decision about what the test guarantees, which is why I have left it alone. Happy to push whichever you prefer.

Worth noting this failure predates this branch: it is on main, and the linker crash was the only reason nobody had seen it.

This branch has not been deployed

No deployments
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