BINIUS-347: stop computing wires the optimiser already dropped - #1908
Open
tcoratger wants to merge 1 commit into
Open
Conversation
Compiling a circuit produces two artifacts: the constraints a proof checks, and the instructions that fill in the witness. Two clean-up passes run before the constraints are emitted, one collapsing duplicated work and one dropping work nothing reads. Both fed the constraints only, so every proof still computed values no constraint mentions. The committed statistics showed it: gate count and instruction count were equal to the digit in every circuit. The dropped set is now computed once and handed to both. Assertions are the exception: one produces no value, so nothing reads it by definition, and its instruction is what reports a failing witness. This surfaces a bug in the duplicate-collapsing pass. It rewrites every reader of a dropped value onto the surviving copy, but the early exit for hint gates sat before that rewrite, so a hint kept pointing at a wire whose defining gate no longer emits a constraint. That was invisible only because the dropped gate was still evaluated. Canonicalising every gate's wires before excluding hints from collapse fixes it. Around forty tests built a circuit, left the result neither asserted nor committed, then read an intermediate value. Several passed for the wrong reason: one asserts a sum and carry are zero, which any all-zero buffer satisfies. Each now pins what it inspects, so the value is constrained rather than merely computed. Constraints, constants and the committed layout are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jimpo
reviewed
Jul 26, 2026
jimpo
left a comment
Collaborator
There was a problem hiding this comment.
Hmm, I don't think it's a good pattern to use force commit this much, even in the tests. It's really a hack to hint at better compilation, not something that should be so important.
The better pattern would be to make inout wires for the outputs and use assert_eq to bind the gate outputs to the inout wires. But it would be annoying to have to then separately compute the expected outputs just to assert_eq. Maybe we should add a mechanism like assign_inout, which takes an inout wire and a private wire, and copies the value to the inout and adds a linear constraint between them (with dst = inout wire).
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.
Closes BINIUS-347.
Two optimiser passes already delete redundant work from the constraints. The witness generator never heard about them, so every proof still computed values no constraint mentions. It does not any more — witness filling drops 21% on zklogin.
Also fixes a latent bug in the duplicate-collapsing pass that this change exposes.
The problem
Compiling a circuit produces two artifacts:
Two clean-up passes run before the constraints are emitted.
One collapses duplicated work, the other drops work nothing reads.
Both fed the constraints only.
The instruction stream never heard about either.
The committed statistics showed it in plain sight — gate count and instruction count equal to the digit:
If anything were being skipped, those two numbers would differ.
The idea
Compute the dropped set once and hand it to both.
An assertion is the one exception.
It produces no value, so nothing reads it by definition.
Its instruction is the only thing that reports a failing witness, so it keeps running even when its constraint is redundant.
Worked example
A latent bug this exposes
The duplicate-collapsing pass rewrites every reader of a dropped value onto the surviving copy.
The early exit for hint gates sat before that rewrite rather than after it.
So a hint kept pointing at a wire whose defining gate no longer emits a constraint.
That is invisible today only because the dropped gate is still evaluated.
Stop evaluating it and the hint reads zero, and every circuit that divides or inverts fails to populate.
for gate in gate_ids { - if matches!(graph.gate_data(gate).opcode, Opcode::Hint) { continue; } - for wire in graph.gates[gate].wires.iter_mut() { /* canonicalise */ } + + if matches!(graph.gate_data(gate).opcode, Opcode::Hint) { continue; }Canonicalise every gate, then exclude hints from being collapse candidates.
That alone fixed 7 of the 12 remaining test failures, and has its own regression test.
What the existing tests revealed
Around forty tests build a circuit, leave the result neither asserted nor committed, then read an intermediate value back out of the witness.
Several passed for the wrong reason.
One asserts a sum and a carry are both zero, which any all-zero buffer satisfies, and its constraint check ran against a system holding no constraint about those wires.
Each now pins what it inspects, so the value is constrained rather than merely computed.
Soundness
Constraints, constants and the committed layout are untouched.
Only the instruction stream shrinks, and only by instructions writing values no constraint names.
The skipped set is closed under reading: any gate whose output a kept gate reads is itself kept, because the liveness pass marks the definer of every input of a live gate.
Scope
Verification
cargo check --workspace --all-targets— cleanclippy -p binius-frontend -p binius-circuits --all-targets --all-features -D warnings— cleanfmt --all --check— cleanbinius-frontend159 tests,binius-circuits320 tests — passBenchmark
Instructions removed, from the snapshot diff:
Wall time, measuring
populate_wire_witnessalone, with the two core files reverted in place for the baseline:Time tracks instruction count almost exactly.
Hand-written hash gadgets are already tight, so they barely move.
The win is in large composed circuits, where redundancy accumulates in the seams between gadgets — and those are the circuits where witness generation costs real time.
blake3_compressreaching zero is not caused by this change: that example already had 0 AND constraints on main, since everything it computes is neither asserted nor committed. The change only makes its witness generation agree with its constraint system. Worth a follow-up to make that example prove something.Benchmark source used to produce the timings
Not part of this change. Drop in as
crates/examples/benches/witness_fill.rs, add a[[bench]]entry withharness = false, and runcargo bench --bench witness_fill -p binius-examples.🤖 Generated with Claude Code