Introduce independent GKR proving module with clean-room sumcheck implementation#248
Open
HudsonGraeme wants to merge 5 commits intomainfrom
Open
Introduce independent GKR proving module with clean-room sumcheck implementation#248HudsonGraeme wants to merge 5 commits intomainfrom
HudsonGraeme wants to merge 5 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds two new workspace crates: Changes
Sequence DiagramssequenceDiagram
autonumber
actor Client
participant Prover as Prover
participant Circuit as LayeredCircuit
participant Sumcheck as Sumcheck
participant Transcript as Sha256Transcript
Client->>Prover: prove(circuit, witness, transcript)
Prover->>Circuit: evaluate(witness)
Circuit-->>Prover: layer outputs
loop per layer
Prover->>Prover: build_eq_table(rz)
Prover->>Prover: assemble bk_f/bk_hg
Prover->>Sumcheck: prove_sumcheck(bk_f, bk_hg, num_vars, transcript)
Sumcheck->>Transcript: append round polys
Transcript-->>Sumcheck: sample challenges
Sumcheck-->>Prover: (proof, challenges)
Prover->>Prover: evaluate_mle(next_layer, challenges)
Prover->>Transcript: append claimed value
end
Prover->>Transcript: finalize_proof()
Transcript-->>Prover: Proof
Prover-->>Client: Proof
sequenceDiagram
autonumber
actor Client
participant Verifier as Verifier
participant Proof as ProofData
participant Transcript as Sha256Transcript
participant Sumcheck as Sumcheck
participant Circuit as LayeredCircuit
Client->>Verifier: verify(circuit, witness, proof, transcript)
Verifier->>Proof: read initial claimed value
Proof-->>Verifier: claimed_v
Verifier->>Transcript: append claimed_v
loop per layer
Verifier->>Proof: read per-layer claimed_val
Proof-->>Verifier: claimed_val
Verifier->>Transcript: append claimed_val
loop per round
Verifier->>Proof: read (p0,p1,p2)
Proof-->>Verifier: round polys
Verifier->>Transcript: append coefficients
Transcript-->>Verifier: sample challenge
Verifier->>Sumcheck: verify round via interpolation
end
Verifier->>Proof: read vy (if present)
Proof-->>Verifier: vy
end
Verifier->>Circuit: evaluate(witness)
Circuit-->>Verifier: witness evaluations
Verifier->>Verifier: compare expected vs claimed
Verifier-->>Client: boolean (valid/invalid)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lementation Develop jst_gkr_engine and jst_gkr crates implementing the GKR interactive proof protocol from published specifications (GKR JACM 2015, Thaler 2013, LFKN 1992). Implement multilinear extension utilities, degree-2 sumcheck prover and verifier with Thaler bookkeeping tables, SHA-256 Fiat-Shamir transcript, and layered circuit evaluation with gate-list wiring.
HudsonGraeme
force-pushed
the
introduce/clean-room-gkr-proving-system
branch
from
2f23104 to
14d9bc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
prover/jst_gkr_engine/src/lib.rs (1)
107-129: Moveuse std::fmt::Debugto the top of the file.The
Debugimport at line 129 is used by thePolynomialCommitmenttrait at line 109. While Rust allows imports anywhere in a file, placing imports at the top improves readability.Suggested fix
Move
use std::fmt::Debug;to line 4 (afteruse arith::Field;):use arith::Field; +use std::fmt::Debug; #[derive(Clone, Debug)]And remove from line 129:
-use std::fmt::Debug;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr_engine/src/lib.rs` around lines 107 - 129, The import use std::fmt::Debug is declared after the PolynomialCommitment trait but is used by that trait; move the import to the top of the file alongside other imports (e.g., immediately after use arith::Field) so Debug is in scope before the trait definition, and remove the trailing use std::fmt::Debug; declaration near the bottom; ensure PolynomialCommitment, Field, and ProofSystem remain unchanged.prover/jst_gkr/Cargo.toml (2)
10-10:randis listed in both[dependencies]and[dev-dependencies].Since
randis already a runtime dependency (line 10), it's automatically available for tests. The duplicate entry in[dev-dependencies](line 13) is redundant.Suggested fix
[dev-dependencies] -rand.workspace = true rand_chacha.workspace = true goldilocks.workspace = trueAlso applies to: 13-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/Cargo.toml` at line 10, Duplicate dependency declaration: "rand" appears in both [dependencies] and [dev-dependencies]; remove the redundant entry. Edit Cargo.toml and delete the "rand" line under the [dev-dependencies] section so "rand" remains only as a runtime dependency in [dependencies]; ensure no other duplicate entries exist and run cargo check to confirm the manifest is valid.
17-18: Same Clippy lint suppression concern asjst_gkr_engine.Consider enabling lints for this new crate to maintain code quality. See the comment on
prover/jst_gkr_engine/Cargo.tomlfor details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/Cargo.toml` around lines 17 - 18, The crate disables all Clippy lints via the [lints.clippy] all = "allow" entry in prover/jst_gkr's Cargo.toml; revert this suppression by removing or tightening that setting (e.g., change all = "warn" or delete the entry) so clippy scans this crate like others, and follow the same lint policy applied to prover/jst_gkr_engine (enable recommended lints or explicitly allow only necessary ones) to maintain consistent code-quality checks across crates.prover/jst_gkr/src/prover.rs (1)
61-61: Thelet _ = sc_proof;suppresses an unused variable warning.While this is intentional (the proof data is already written to the transcript via
prove_sumcheck), a brief comment would clarify this design choice for future readers.Suggested clarification
- let _ = sc_proof; + // sc_proof data is already appended to transcript; struct not needed separately + let _ = sc_proof;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/prover.rs` at line 61, The unused binding let _ = sc_proof; intentionally suppresses a warning because the proof bytes are already consumed/written into the transcript by prove_sumcheck, so sc_proof is purposefully unused thereafter; update the code around sc_proof in prover.rs to replace or accompany that statement with a short clarifying comment referencing prove_sumcheck and transcript (e.g., "sc_proof consumed by prove_sumcheck and recorded into transcript, kept only to avoid unused-variable warnings") so future readers understand the design choice.prover/jst_gkr_engine/Cargo.toml (1)
9-10: Disabling all Clippy lints undermines code quality for a new crate.Setting
all = "allow"silences all Clippy warnings at the crate level. For a new, auditable implementation (as described in the PR objectives), enabling lints would help catch issues early. Consider removing this section or selectively allowing only specific lints that are intentionally ignored.Suggested fix
-[lints.clippy] -all = "allow"Or, if specific lints need suppression, allow them explicitly:
[lints.clippy] # Example: allow specific lints with justification module_name_repetitions = "allow"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr_engine/Cargo.toml` around lines 9 - 10, The crate-level Clippy suppression '[lints.clippy] all = "allow"' disables all lints; remove that section or replace it by explicitly allowing only the needed lint names (e.g., keep '[lints.clippy]' and list specific lints like 'module_name_repetitions = "allow"' instead of 'all = "allow"') so Clippy warnings remain enabled for the rest of the crate; update Cargo.toml to either delete the 'lints.clippy' block or narrow it to explicit lint keys to be suppressed.prover/jst_gkr/src/tests.rs (2)
116-136: Test name says "two_layer" but circuit has only one layer.The function is named
test_gkr_two_layer_circuit, but the circuit is constructed withlayers: vec![layer]— a single layer. Consider renaming for clarity:-fn test_gkr_two_layer_circuit() { +fn test_gkr_single_layer_circuit() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/tests.rs` around lines 116 - 136, The test function test_gkr_two_layer_circuit constructs a LayeredCircuit with only one CircuitLayer (layers: vec![layer]), causing a name/behavior mismatch; either rename the test to reflect a single-layer circuit or add a second CircuitLayer to the LayeredCircuit (e.g., create another CircuitLayer instance and use layers: vec![layer, second_layer]) so the test actually covers two layers—update the function name or the LayeredCircuit.layers content accordingly, referencing test_gkr_two_layer_circuit, CircuitLayer, and LayeredCircuit.
150-179: Consider extending to test prove/verify for addition-only circuits.This test validates circuit evaluation but doesn't exercise the proving/verification path. Addition-only circuits may exercise different code paths than multiplication circuits, so testing the full protocol would improve coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/tests.rs` around lines 150 - 179, Extend the test_gkr_addition_circuit to exercise the full prove/verify flow: construct the same addition-only LayeredCircuit (using CircuitLayer, AddGate, LayeredCircuit) and witness, then invoke the library's GKR proving routine (e.g., Prover::prove or the module's prove function) to produce a proof and pass that proof into the verifier routine (e.g., Verifier::verify or verify_proof) asserting verification succeeds; ensure any required public inputs or configuration used by the prover/verifier are supplied so the addition-only path is actually executed.prover/jst_gkr/src/lib.rs (1)
1-1: Overly broad lint suppression —clippy::alldisables useful warnings.
#![allow(clippy::all)]suppresses all Clippy lints, including correctness-related ones. Consider being more selective:-#![allow(clippy::pedantic, clippy::all)] +#![allow(clippy::pedantic)]Or allow specific lints that are causing noise rather than blanket-disabling all of them. This is especially important for a cryptographic proving system where correctness is critical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/lib.rs` at line 1, The crate-level attribute "#![allow(clippy::pedantic, clippy::all)]" is too broad and mutes important Clippy checks; replace it by removing "clippy::all" and either delete the attribute entirely or selectively allow only the specific lints causing noise (e.g., keep "#![allow(clippy::pedantic)]" if desired or list precise lints like "clippy::module_name_repetitions" etc.); update the attribute in lib.rs so that only targeted clippy rules are suppressed rather than using the blanket "clippy::all".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prover/jst_gkr/src/prover.rs`:
- Around line 51-54: The computed const_contribution (from layer.const_gates
using eq_table and g.coef) is never applied to the proof claim; update the code
that prepares the sumcheck claim (the variable claimed_val used before invoking
the sumcheck routine) to incorporate const_contribution (add or subtract
according to the protocol variant) so the sumcheck proves the full circuit
value; ensure you reference the same symbols: const_contribution, eq_table,
layer.const_gates, claimed_val, and the sumcheck invocation so the constant-gate
term is included in the claimed polynomial before running the sumcheck.
In `@prover/jst_gkr/src/tests.rs`:
- Around line 145-148: The test currently only calls prove(...) and asserts
proof.data is non-empty but never calls verify(...), so update the test to
import and call verify (e.g., verify::<F, Sha256Transcript>(&circuit, &proof,
&mut verifier_transcript)) after proof creation and assert verification
succeeds; create a verifier transcript (Sha256Transcript::default()), pass the
same circuit and the produced proof to verify, and replace the existing
non-empty-data assert with an assertion that verify returns Ok(true) or
equivalent success result; ensure you add the verify import alongside prove and
Sha256Transcript to the test module.
In `@prover/jst_gkr/src/transcript.rs`:
- Around line 33-39: The function challenge_field_element currently feeds a
single 32-byte SHA-256 digest into F::from_uniform_bytes, which fails for
extension fields (e.g., FrxN) and can bias BN254; update challenge_field_element
to derive as many bytes as F::from_uniform_bytes requires by using an XOF (e.g.,
SHAKE256) or an iterative HMAC/SHA256-expand loop that appends successive hashes
to produce 32*N (or the exact length requested) before calling
F::from_uniform_bytes; keep the same transcript state update semantics (advance
state with the bytes consumed) and ensure the expansion is deterministic from
self.state so challenge_field_element returns uniformly-sized byte arrays for
all Field implementations.
In `@prover/jst_gkr/src/verifier.rs`:
- Around line 67-72: The helper read_field_from_proof currently slices data
without checking bounds which can panic on truncated proofs; modify
read_field_from_proof (and callers) to first verify that data.len() >= *offset +
F::SIZE and, if not, return an error or signal failure to the verifier (e.g.,
make the function return Result<F, VerifyError> or Option<F>), adjust call sites
in verify to propagate that failure and return early (false) on insufficient
proof data so malformed proofs do not cause a panic.
- Around line 49-57: The Sumcheck proof (sc_proof) is constructed but never
verified and the verifier discards sampled challenges by overwriting r_x with
zeros; replace the discard and zeroing by calling the sumcheck verification
routine (e.g., verify_sumcheck) using claimed_val, &sc_proof, input_var_num and
the transcript (after reading vy via read_field_from_proof and
transcript.append_field_element), assert the verification result, and set r_x to
the challenge vector returned by verify_sumcheck (rather than vec![F::ZERO;
input_var_num]) so subsequent layers use the sampled challenges; ensure any
verification failure is handled (panic/error) consistent with surrounding code.
- Around line 45-46: The sampled challenge is being discarded (let _r: F =
transcript.challenge_field_element()) but must be stored and fed into the next
layer; replace the underscore binding with a real variable (e.g. let r =
transcript.challenge_field_element()) and append/assign it to the accumulator
used for next-layer challenges (update r_x or the vector that holds layer
challenges) so that the sampled r is preserved alongside pushing round_polys
([p0,p1,p2]) and later used in the next verification step.
---
Nitpick comments:
In `@prover/jst_gkr_engine/Cargo.toml`:
- Around line 9-10: The crate-level Clippy suppression '[lints.clippy] all =
"allow"' disables all lints; remove that section or replace it by explicitly
allowing only the needed lint names (e.g., keep '[lints.clippy]' and list
specific lints like 'module_name_repetitions = "allow"' instead of 'all =
"allow"') so Clippy warnings remain enabled for the rest of the crate; update
Cargo.toml to either delete the 'lints.clippy' block or narrow it to explicit
lint keys to be suppressed.
In `@prover/jst_gkr_engine/src/lib.rs`:
- Around line 107-129: The import use std::fmt::Debug is declared after the
PolynomialCommitment trait but is used by that trait; move the import to the top
of the file alongside other imports (e.g., immediately after use arith::Field)
so Debug is in scope before the trait definition, and remove the trailing use
std::fmt::Debug; declaration near the bottom; ensure PolynomialCommitment,
Field, and ProofSystem remain unchanged.
In `@prover/jst_gkr/Cargo.toml`:
- Line 10: Duplicate dependency declaration: "rand" appears in both
[dependencies] and [dev-dependencies]; remove the redundant entry. Edit
Cargo.toml and delete the "rand" line under the [dev-dependencies] section so
"rand" remains only as a runtime dependency in [dependencies]; ensure no other
duplicate entries exist and run cargo check to confirm the manifest is valid.
- Around line 17-18: The crate disables all Clippy lints via the [lints.clippy]
all = "allow" entry in prover/jst_gkr's Cargo.toml; revert this suppression by
removing or tightening that setting (e.g., change all = "warn" or delete the
entry) so clippy scans this crate like others, and follow the same lint policy
applied to prover/jst_gkr_engine (enable recommended lints or explicitly allow
only necessary ones) to maintain consistent code-quality checks across crates.
In `@prover/jst_gkr/src/lib.rs`:
- Line 1: The crate-level attribute "#![allow(clippy::pedantic, clippy::all)]"
is too broad and mutes important Clippy checks; replace it by removing
"clippy::all" and either delete the attribute entirely or selectively allow only
the specific lints causing noise (e.g., keep "#![allow(clippy::pedantic)]" if
desired or list precise lints like "clippy::module_name_repetitions" etc.);
update the attribute in lib.rs so that only targeted clippy rules are suppressed
rather than using the blanket "clippy::all".
In `@prover/jst_gkr/src/prover.rs`:
- Line 61: The unused binding let _ = sc_proof; intentionally suppresses a
warning because the proof bytes are already consumed/written into the transcript
by prove_sumcheck, so sc_proof is purposefully unused thereafter; update the
code around sc_proof in prover.rs to replace or accompany that statement with a
short clarifying comment referencing prove_sumcheck and transcript (e.g.,
"sc_proof consumed by prove_sumcheck and recorded into transcript, kept only to
avoid unused-variable warnings") so future readers understand the design choice.
In `@prover/jst_gkr/src/tests.rs`:
- Around line 116-136: The test function test_gkr_two_layer_circuit constructs a
LayeredCircuit with only one CircuitLayer (layers: vec![layer]), causing a
name/behavior mismatch; either rename the test to reflect a single-layer circuit
or add a second CircuitLayer to the LayeredCircuit (e.g., create another
CircuitLayer instance and use layers: vec![layer, second_layer]) so the test
actually covers two layers—update the function name or the LayeredCircuit.layers
content accordingly, referencing test_gkr_two_layer_circuit, CircuitLayer, and
LayeredCircuit.
- Around line 150-179: Extend the test_gkr_addition_circuit to exercise the full
prove/verify flow: construct the same addition-only LayeredCircuit (using
CircuitLayer, AddGate, LayeredCircuit) and witness, then invoke the library's
GKR proving routine (e.g., Prover::prove or the module's prove function) to
produce a proof and pass that proof into the verifier routine (e.g.,
Verifier::verify or verify_proof) asserting verification succeeds; ensure any
required public inputs or configuration used by the prover/verifier are supplied
so the addition-only path is actually executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ac7e996-86e8-453f-8172-d624fbec48fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlprover/jst_gkr/Cargo.tomlprover/jst_gkr/src/lib.rsprover/jst_gkr/src/mle.rsprover/jst_gkr/src/prover.rsprover/jst_gkr/src/sumcheck.rsprover/jst_gkr/src/tests.rsprover/jst_gkr/src/transcript.rsprover/jst_gkr/src/verifier.rsprover/jst_gkr_engine/Cargo.tomlprover/jst_gkr_engine/src/lib.rs
Comment on lines
+33
to
+39
| fn challenge_field_element<F: Field>(&mut self) -> F { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(&self.state); | ||
| let hash = hasher.finalize(); | ||
| self.state = hash.to_vec(); | ||
| F::from_uniform_bytes(&hash) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what from_uniform_bytes implementations exist and their byte requirements
echo "=== Checking from_uniform_bytes implementations ==="
rg -n "fn from_uniform_bytes" --type rust -A 5 prover/arith/Repository: inference-labs-inc/JSTprove
Length of output: 16952
SHA-256 output (32 bytes) is insufficient for F::from_uniform_bytes on extension fields.
The challenge_field_element function passes a 32-byte SHA-256 hash directly to F::from_uniform_bytes. The codebase's field implementations have varying byte requirements:
- Extension fields (e.g.,
FrxNfor N>1) require32*Nbytes and panic when given only 32 bytes (line 167 inprover/arith/src/bn254xn.rs:assert!(bytes.len() >= 32 * N)). - BN254 requires >= 32 bytes and pads input with zeros to reach 64 bytes (lines 81-82 in
prover/arith/src/bn254.rs), which may introduce bias.
For a Fiat-Shamir transcript used in cryptographic protocols, this causes runtime failures for extension fields and potential bias in challenges for BN254.
Suggested fix: Use an expandable hash or iterative hashing
fn challenge_field_element<F: Field>(&mut self) -> F {
- let mut hasher = Sha256::new();
- hasher.update(&self.state);
- let hash = hasher.finalize();
- self.state = hash.to_vec();
- F::from_uniform_bytes(&hash)
+ let mut output = Vec::new();
+ let mut counter = 0u64;
+ while output.len() < 64 {
+ let mut hasher = Sha256::new();
+ hasher.update(&self.state);
+ hasher.update(&counter.to_le_bytes());
+ output.extend_from_slice(&hasher.finalize());
+ counter += 1;
+ }
+ self.state = output[..32].to_vec();
+ F::from_uniform_bytes(&output)
}Alternatively, use a XOF like SHAKE256 for arbitrary-length output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prover/jst_gkr/src/transcript.rs` around lines 33 - 39, The function
challenge_field_element currently feeds a single 32-byte SHA-256 digest into
F::from_uniform_bytes, which fails for extension fields (e.g., FrxN) and can
bias BN254; update challenge_field_element to derive as many bytes as
F::from_uniform_bytes requires by using an XOF (e.g., SHAKE256) or an iterative
HMAC/SHA256-expand loop that appends successive hashes to produce 32*N (or the
exact length requested) before calling F::from_uniform_bytes; keep the same
transcript state update semantics (advance state with the bytes consumed) and
ensure the expansion is deterministic from self.state so challenge_field_element
returns uniformly-sized byte arrays for all Field implementations.
prover/jst_gkr/src/verifier.rs
Outdated
Comment on lines
+45
to
+46
| let _r: F = transcript.challenge_field_element(); | ||
| round_polys.push([p0, p1, p2]); |
There was a problem hiding this comment.
Sampled challenge _r is discarded — should be collected for verification.
The challenge _r is sampled (which keeps the transcript synchronized with the prover), but the underscore prefix and lack of collection suggest it's being ignored. These challenges should be accumulated and used as the new r_x for the next layer, rather than being discarded.
+ let mut challenges = Vec::with_capacity(input_var_num);
for _ in 0..input_var_num {
// ... read p0, p1, p2 ...
- let _r: F = transcript.challenge_field_element();
+ let r: F = transcript.challenge_field_element();
+ challenges.push(r);
round_polys.push([p0, p1, p2]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let _r: F = transcript.challenge_field_element(); | |
| round_polys.push([p0, p1, p2]); | |
| let mut challenges = Vec::with_capacity(input_var_num); | |
| for _ in 0..input_var_num { | |
| // ... read p0, p1, p2 ... | |
| let r: F = transcript.challenge_field_element(); | |
| challenges.push(r); | |
| round_polys.push([p0, p1, p2]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prover/jst_gkr/src/verifier.rs` around lines 45 - 46, The sampled challenge
is being discarded (let _r: F = transcript.challenge_field_element()) but must
be stored and fed into the next layer; replace the underscore binding with a
real variable (e.g. let r = transcript.challenge_field_element()) and
append/assign it to the accumulator used for next-layer challenges (update r_x
or the vector that holds layer challenges) so that the sampled r is preserved
alongside pushing round_polys ([p0,p1,p2]) and later used in the next
verification step.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prover/jst_gkr/src/prover.rs (1)
58-61: Consider using_sc_proofnaming instead of explicit discard.The
let _ = sc_proof;pattern works but is unconventional. Sinceprove_sumcheckembeds round polynomials directly into the transcript, the return value is intentionally unused. Using_sc_proofas the binding name is more idiomatic Rust for intentionally unused variables.♻️ Suggested change
- let (sc_proof, challenges) = + let (_sc_proof, challenges) = prove_sumcheck(&mut bk_f, &mut bk_hg, input_var_num, transcript); - - let _ = sc_proof;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/prover.rs` around lines 58 - 61, Replace the explicit discard pattern by binding the unused sumcheck proof with an underscore-prefixed name; specifically, change the destructuring that currently binds sc_proof from the call to prove_sumcheck(...) so the tuple is assigned to (_sc_proof, challenges) instead of (sc_proof, challenges) followed by let _ = sc_proof;, so the proof return value is clearly and idiomatically marked unused while keeping challenges in scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prover/jst_gkr/src/prover.rs`:
- Around line 58-61: Replace the explicit discard pattern by binding the unused
sumcheck proof with an underscore-prefixed name; specifically, change the
destructuring that currently binds sc_proof from the call to prove_sumcheck(...)
so the tuple is assigned to (_sc_proof, challenges) instead of (sc_proof,
challenges) followed by let _ = sc_proof;, so the proof return value is clearly
and idiomatically marked unused while keeping challenges in scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca1b420d-62f4-4fd6-83f7-2b0be71d9706
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlprover/jst_gkr/Cargo.tomlprover/jst_gkr/src/lib.rsprover/jst_gkr/src/mle.rsprover/jst_gkr/src/prover.rsprover/jst_gkr/src/sumcheck.rsprover/jst_gkr/src/tests.rsprover/jst_gkr/src/transcript.rsprover/jst_gkr/src/verifier.rsprover/jst_gkr_engine/Cargo.tomlprover/jst_gkr_engine/src/lib.rs
✅ Files skipped from review due to trivial changes (6)
- prover/jst_gkr/src/lib.rs
- prover/jst_gkr/Cargo.toml
- prover/jst_gkr_engine/Cargo.toml
- Cargo.toml
- prover/jst_gkr/src/tests.rs
- prover/jst_gkr_engine/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- prover/jst_gkr/src/mle.rs
- prover/jst_gkr/src/transcript.rs
- prover/jst_gkr/src/verifier.rs
- prover/jst_gkr/src/sumcheck.rs
Implement two-phase sumcheck per GKR layer for mul gate verification, replacing the unsound single-phase approach that double-counted gate contributions. Phase 1 reduces x-variables with one-sided accumulation, phase 2 verifies y-variables independently, and dual claims combine via random linear combination (alpha) for next-layer propagation. Introduce verifier sumcheck validation with full round-polynomial consistency checks, correct challenge propagation, and final wiring predicate evaluation against the circuit structure. Add bounds-checked proof deserialization returning Option to reject truncated proofs without panicking. Incorporate constant gate contributions into the sumcheck claim by subtracting from the combined layer output before phase 1. Expand Fiat-Shamir transcript challenge derivation to 64 bytes via iterative SHA-256 counter mode to eliminate sampling bias for larger fields. Extend test coverage with 11 tests: single-layer mul+add, addition-only, two-layer deep circuit, constant gates, wrong-witness rejection, and truncated proof rejection. Remove blanket clippy suppression across both crates.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
prover/jst_gkr/src/lib.rs (1)
1-1: Consider narrowing the clippy suppression.The PR objectives mention "blanket clippy suppressions removed," but
#![allow(clippy::pedantic)]remains. While suppressingpedanticis common (as it's intentionally strict), consider either:
- Documenting why pedantic is suppressed at the crate level
- Using targeted
#[allow(...)]on specific items that trigger pedantic lintsThis is a minor concern as
pedanticlints are advisory, not correctness issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/lib.rs` at line 1, The crate currently suppresses all pedantic clippy lints via the crate attribute #![allow(clippy::pedantic)]; narrow this by either removing that blanket attribute and adding targeted #[allow(...)] annotations on the specific functions/modules that trigger pedantic lints (identify offending lints by running cargo clippy and apply e.g. #[allow(clippy::xxx)] on the exact functions or impls), or keep the crate-level suppression but add a crate-level doc comment explaining why clippy::pedantic is globally suppressed; update the attribute to the chosen approach and re-run clippy to ensure only the intended lints remain suppressed.prover/jst_gkr/src/prover.rs (1)
51-68:_phase1_sumis computed but unused — either dead code or incomplete implementation.The
const_contributionis correctly computed and subtracted fromcombined_claimto get_phase1_sum, but this value is never used. The sumcheck proceeds withbk_fandbk_hgwithout any reference to the expected sum.The verifier (lines 56-62 of verifier.rs) uses
phase1_sumas theclaimed_sumargument toverify_sumcheck. For soundness, the prover's sumcheck output and the verifier's expectedphase1_summust match. This works because the sumcheck naturally computessum(bk_hg * bk_f)which should equalphase1_sumwhen the gate structures are correct.However, the unused
_phase1_sumvariable is confusing and suggests incomplete code. Consider either:
- Removing it if it's truly dead code
- Adding an assertion
debug_assert_eq!(/* sumcheck claimed sum */, _phase1_sum)for debugging- Documenting why it's computed but not explicitly used
♻️ Suggested cleanup: remove or document the unused variable
- let _phase1_sum = combined_claim - const_contribution; + // The sumcheck will prove: sum(bk_hg[x] * bk_f[x]) over all x + // This should equal (combined_claim - const_contribution) by construction🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/prover.rs` around lines 51 - 68, The computed _phase1_sum (const_contribution = combined_claim - _phase1_sum) is unused and should either be removed or tied to the sumcheck: either delete the _phase1_sum computation (remove const_contribution/ _phase1_sum) or use it to validate the sumcheck by computing the actual claimed sum from bk_f and bk_hg and asserting equality (e.g., compute actual_sum = sum(bk_hg[i] * bk_f[i]) and debug_assert_eq!(actual_sum, _phase1_sum)) before calling prove_sumcheck, or if prove_sumcheck accepts a claimed_sum, pass _phase1_sum as that argument; reference variables/functions: _phase1_sum, const_contribution, combined_claim, bk_f, bk_hg, prove_sumcheck.prover/jst_gkr/benches/comparison.rs (1)
186-208: Clean-room GKR benchmark lacks verification — consider adding a correctness check.The Expander GKR benchmark (lines 153-161) includes a verification assertion to ensure proof correctness before timing. The clean-room GKR benchmark only times
jst_provewithout any verification. Adding a verification check would ensure the benchmark measures valid proofs.♻️ Suggested: add verification check
{ let (jst_circuit, witness) = build_jst_circuit(depth, log_size); + // Verify correctness once before benchmarking + { + let mut t = Sha256Transcript::default(); + let proof = jst_prove::<Goldilocks, Sha256Transcript>(&jst_circuit, &witness, &mut t); + let mut v = Sha256Transcript::default(); + assert!(jst_gkr::verifier::verify::<Goldilocks, Sha256Transcript>(&jst_circuit, &witness, &proof, &mut v)); + } + for _ in 0..warmup {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/benches/comparison.rs` around lines 186 - 208, The benchmark currently calls jst_prove::<Goldilocks, Sha256Transcript>(&jst_circuit, &witness, &mut t) but never verifies the produced proof; add calls to the verification routine (e.g., jst_verify::<Goldilocks, Sha256Transcript> or the project’s verify function) to check correctness both in the warmup loop and for each measured iteration, ensuring the verification runs outside the timed region so timings still measure only jst_prove; assert the verification result is OK to fail fast on invalid proofs and reference build_jst_circuit, jst_prove, Sha256Transcript, and the verify function when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prover/jst_gkr/src/sumcheck.rs`:
- Around line 56-82: The verifier unconditionally indexes proof.round_polys in
verify_sumcheck causing potential OOB panics for truncated proofs; add an
explicit bounds check before the loop or at each iteration: ensure
proof.round_polys.len() >= num_vars (or that round < proof.round_polys.len())
and return None (or an Err variant) if the proof is too short, then proceed to
use poly = &proof.round_polys[round] and the existing logic (keeping identifiers
verify_sumcheck, proof.round_polys, num_vars, round) intact.
---
Nitpick comments:
In `@prover/jst_gkr/benches/comparison.rs`:
- Around line 186-208: The benchmark currently calls jst_prove::<Goldilocks,
Sha256Transcript>(&jst_circuit, &witness, &mut t) but never verifies the
produced proof; add calls to the verification routine (e.g.,
jst_verify::<Goldilocks, Sha256Transcript> or the project’s verify function) to
check correctness both in the warmup loop and for each measured iteration,
ensuring the verification runs outside the timed region so timings still measure
only jst_prove; assert the verification result is OK to fail fast on invalid
proofs and reference build_jst_circuit, jst_prove, Sha256Transcript, and the
verify function when making the change.
In `@prover/jst_gkr/src/lib.rs`:
- Line 1: The crate currently suppresses all pedantic clippy lints via the crate
attribute #![allow(clippy::pedantic)]; narrow this by either removing that
blanket attribute and adding targeted #[allow(...)] annotations on the specific
functions/modules that trigger pedantic lints (identify offending lints by
running cargo clippy and apply e.g. #[allow(clippy::xxx)] on the exact functions
or impls), or keep the crate-level suppression but add a crate-level doc comment
explaining why clippy::pedantic is globally suppressed; update the attribute to
the chosen approach and re-run clippy to ensure only the intended lints remain
suppressed.
In `@prover/jst_gkr/src/prover.rs`:
- Around line 51-68: The computed _phase1_sum (const_contribution =
combined_claim - _phase1_sum) is unused and should either be removed or tied to
the sumcheck: either delete the _phase1_sum computation (remove
const_contribution/ _phase1_sum) or use it to validate the sumcheck by computing
the actual claimed sum from bk_f and bk_hg and asserting equality (e.g., compute
actual_sum = sum(bk_hg[i] * bk_f[i]) and debug_assert_eq!(actual_sum,
_phase1_sum)) before calling prove_sumcheck, or if prove_sumcheck accepts a
claimed_sum, pass _phase1_sum as that argument; reference variables/functions:
_phase1_sum, const_contribution, combined_claim, bk_f, bk_hg, prove_sumcheck.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1875c89-0e5f-42c1-bc76-5bfd907ab37d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
prover/jst_gkr/Cargo.tomlprover/jst_gkr/benches/comparison.rsprover/jst_gkr/benches/gkr_bench.rsprover/jst_gkr/src/lib.rsprover/jst_gkr/src/mle.rsprover/jst_gkr/src/prover.rsprover/jst_gkr/src/sumcheck.rsprover/jst_gkr/src/tests.rsprover/jst_gkr/src/transcript.rsprover/jst_gkr/src/verifier.rsprover/jst_gkr_engine/Cargo.tomlprover/jst_gkr_engine/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- prover/jst_gkr/Cargo.toml
- prover/jst_gkr/src/tests.rs
- prover/jst_gkr_engine/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- prover/jst_gkr_engine/Cargo.toml
- prover/jst_gkr/src/mle.rs
Add bounds check in verify_sumcheck rejecting proofs with insufficient round polynomials before indexing. Replace needless range loop in test_eq_table_consistency with iterator enumeration to satisfy needless_range_loop lint on CI. Introduce debug_assert validating phase1_sum consistency against the bookkeeping table product sum prior to sumcheck invocation. Add proof verification assertion to comparison benchmark ensuring correctness before timing iterations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
prover/jst_gkr/src/prover.rs (1)
7-15: Expose a lower-level proving entrypoint that accepts precomputed layer values.Line 12 hardwires
circuit.evaluate(witness)into proof construction. That makes the prover less reusable and forces end-to-end timing incomparison.rseven when the other implementation is measuring prove-only work. Keeping this wrapper is fine, but aprove_with_evaluations(...)helper would better preserve the separation-of-concerns goal of this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/prover.rs` around lines 7 - 15, The current prove function always calls circuit.evaluate(witness) which prevents reusing the prover with precomputed layer values; add a new lower-level entrypoint prove_with_evaluations<F: Field, T: FiatShamirTranscript>(circuit: &LayeredCircuit<F>, layer_vals: &[Vec<F>], witness_len: usize, transcript: &mut T) -> Proof (or similar signature) that constructs the proof using the provided layer_vals and circuit.depth() (or accept depth if needed), then refactor the existing prove(...) to call circuit.evaluate(witness) and forward its result into prove_with_evaluations; update references to use prove_with_evaluations where precomputed evaluations are available (keep names prove and prove_with_evaluations to locate implementations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prover/jst_gkr/benches/comparison.rs`:
- Around line 171-176: The benchmark measures different work: in comparison.rs
the timer starts after calling c.evaluate() but jst_prove (prover.prove) calls
circuit.evaluate(witness) internally, so one side measures evaluate+prove and
the other only prove. Fix by making measurement consistent: either move the
Instant::now() to before c.evaluate() so timing covers evaluate() +
prover.prove(&mut c, ...) for both implementations, or ensure you call
c.evaluate() before starting the timer for the jst/prover.prove path as well
(i.e., perform evaluation on the cloned circuit prior to timing both
implementations); apply the same change to the other loop at lines corresponding
to 206-210 so both benchmarks include exactly the same work.
In `@prover/jst_gkr/src/sumcheck.rs`:
- Around line 17-18: The loop starting with "for round in 0..num_vars" computes
eval_size = 1 << (num_vars - 1 - round) and subsequent logic assumes two input
slices contain at least 2^num_vars elements; add an upfront bounds check before
this loop that validates both slices' lengths are >= (1 << num_vars) and return
an Err on failure (i.e., make the containing function return Result<..., Error>
instead of panicking). Update the function signature to be fallible, replace any
implicit panics with early Err returns, and propagate the Result through callers
so the size validation prevents out-of-bounds access in the "for round ..." loop
and the code paths around eval_size.
---
Nitpick comments:
In `@prover/jst_gkr/src/prover.rs`:
- Around line 7-15: The current prove function always calls
circuit.evaluate(witness) which prevents reusing the prover with precomputed
layer values; add a new lower-level entrypoint prove_with_evaluations<F: Field,
T: FiatShamirTranscript>(circuit: &LayeredCircuit<F>, layer_vals: &[Vec<F>],
witness_len: usize, transcript: &mut T) -> Proof (or similar signature) that
constructs the proof using the provided layer_vals and circuit.depth() (or
accept depth if needed), then refactor the existing prove(...) to call
circuit.evaluate(witness) and forward its result into prove_with_evaluations;
update references to use prove_with_evaluations where precomputed evaluations
are available (keep names prove and prove_with_evaluations to locate
implementations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4472b85f-27a9-4c11-a476-b6cff5496939
📒 Files selected for processing (4)
prover/jst_gkr/benches/comparison.rsprover/jst_gkr/src/prover.rsprover/jst_gkr/src/sumcheck.rsprover/jst_gkr/src/tests.rs
✅ Files skipped from review due to trivial changes (1)
- prover/jst_gkr/src/tests.rs
Comment on lines
+17
to
+18
| for round in 0..num_vars { | ||
| let eval_size = 1 << (num_vars - 1 - round); |
There was a problem hiding this comment.
Validate the bookkeeping buffer sizes before the first round.
Lines 23-27 and 47-49 assume both slices contain at least 2^num_vars evaluations. A mismatched caller currently gets an out-of-bounds panic instead of a rejected input. Add an upfront size check here, and make this API fallible if malformed inputs are expected to reach it.
Also applies to: 23-27, 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prover/jst_gkr/src/sumcheck.rs` around lines 17 - 18, The loop starting with
"for round in 0..num_vars" computes eval_size = 1 << (num_vars - 1 - round) and
subsequent logic assumes two input slices contain at least 2^num_vars elements;
add an upfront bounds check before this loop that validates both slices' lengths
are >= (1 << num_vars) and return an Err on failure (i.e., make the containing
function return Result<..., Error> instead of panicking). Update the function
signature to be fallible, replace any implicit panics with early Err returns,
and propagate the Result through callers so the size validation prevents
out-of-bounds access in the "for round ..." loop and the code paths around
eval_size.
…surement asymmetry Refactor prove into prove_with_evaluations accepting precomputed layer values, eliminating redundant circuit evaluation when callers already hold intermediate results. Original prove delegates to the new function after evaluating the circuit. Resolve measurement inconsistency in comparison benchmark where the Expander path excluded circuit evaluation from timing while the clean-room path included it. Both paths now time evaluate+prove symmetrically. Add assert guard in prove_sumcheck validating bookkeeping table dimensions against num_vars before indexing into the arrays.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prover/jst_gkr/src/prover.rs (1)
23-31: Consider adding a size assertion foroutput_vals.Line 30 calls
evaluate_mle(output_vals, &output_r), which internally assertsevals.len() == 1 << point.len(). Iflayer_vals[depth]has an unexpected size (due to circuit misconfiguration or incorrectevaluateimplementation), the panic message fromevaluate_mlewon't clearly indicate the source.A local assertion could provide a clearer diagnostic:
🛡️ Suggested defensive check
let output_vals = &layer_vals[depth]; let output_var_num = circuit.layers[0].output_var_num; + debug_assert_eq!( + output_vals.len(), + 1 << output_var_num, + "output_vals size mismatch: expected {}, got {}", + 1 << output_var_num, + output_vals.len() + ); let output_r: Vec<F> = (0..output_var_num)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prover/jst_gkr/src/prover.rs` around lines 23 - 31, Add a defensive size assertion before calling evaluate_mle: check that output_vals.len() == 1 << output_r.len() (where output_vals is layer_vals[depth] and output_r is the challenge vector built from output_var_num) and panic with a clear message including depth, expected size, and actual size; this uses the existing symbols output_vals, layer_vals, depth, output_r, output_var_num, and evaluate_mle to make the failure source obvious.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prover/jst_gkr/src/prover.rs`:
- Around line 23-31: Add a defensive size assertion before calling evaluate_mle:
check that output_vals.len() == 1 << output_r.len() (where output_vals is
layer_vals[depth] and output_r is the challenge vector built from
output_var_num) and panic with a clear message including depth, expected size,
and actual size; this uses the existing symbols output_vals, layer_vals, depth,
output_r, output_var_num, and evaluate_mle to make the failure source obvious.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32d6ee74-bf06-449d-acf4-f4c595a0b646
📒 Files selected for processing (3)
prover/jst_gkr/benches/comparison.rsprover/jst_gkr/src/prover.rsprover/jst_gkr/src/sumcheck.rs
✅ Files skipped from review due to trivial changes (1)
- prover/jst_gkr/benches/comparison.rs
Add precondition check validating output_vals length matches the expected 2^output_var_num before MLE evaluation, providing depth and size context in the panic message for diagnostic clarity.
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
1 participant
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.
Summary
jst_gkr_engine,jst_gkr) implementing the GKR interactive proof protocol from first principles based on the published protocol specifications (Goldwasser-Kalai-Rothblum JACM 2015, Thaler 2013, Lund-Fortnow-Karloff-Nisan 1992).ProofSystem,FiatShamirTranscript,PolynomialCommitment) for modular composition with future PCS backends.This module is designed as an independent, auditable implementation grounded directly in the original protocol specifications, providing improved modularity and soundness guarantees through a clean separation of concerns.
Test plan
cargo clippy -- -D warningspasses on both cratescargo fmt --checkpasses on both cratesSummary by CodeRabbit
New Features
Benchmarks
Tests