Implement Groth16 ZKP circuits for battle and state verification#88
Implement Groth16 ZKP circuits for battle and state verification#88
Conversation
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements production-ready Groth16 zero-knowledge proof circuits for BitCell's consensus verification system, replacing stub implementations with cryptographically secure circuits. The implementation introduces a two-tier architecture where full constraint circuits (with complete CA evolution and Merkle tree verification) become the default exports, while simplified circuits remain available for testing.
Key Changes:
- Implemented
setup(),prove(),verify(), andpublic_inputs()methods for BattleCircuit, StateCircuit, and NullifierCircuit using Groth16 over BN254 curve - Changed default exports to point to full constraint implementations (battle_constraints, state_constraints) with backward-compatible aliases for simplified circuits
- Added comprehensive tests for proof generation and verification workflows
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/bitcell-zkp/src/lib.rs | Updated module documentation to explain two-tier circuit architecture; changed default exports to full constraint implementations; added backward-compatible aliases |
| crates/bitcell-zkp/src/battle_constraints.rs | Implemented Groth16 proof generation/verification API with setup(), prove(), verify(), and public_inputs() helper methods; added integration test |
| crates/bitcell-zkp/src/state_constraints.rs | Implemented Groth16 APIs for StateCircuit and NullifierCircuit with proof generation/verification; added comprehensive tests |
| crates/bitcell-zkp/src/battle_circuit.rs | Updated documentation to clarify simplified circuit usage and reference full implementation |
| crates/bitcell-zkp/src/state_circuit.rs | Updated documentation to clarify simplified circuit usage and reference full implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public_inputs: &[Fr], | ||
| ) -> crate::Result<bool> { | ||
| Groth16::<Bn254>::verify(vk, public_inputs, &proof.proof) | ||
| .map_err(|e| crate::Error::ProofVerification) |
There was a problem hiding this comment.
The error from Groth16::verify() is being discarded. This loses valuable debugging information about why verification failed. Consider preserving the error message like in the prove() method:
.map_err(|e| crate::Error::ProofVerification(e.to_string()))Note: This would require updating the Error::ProofVerification variant to accept a String parameter.
| .map_err(|e| crate::Error::ProofVerification) | |
| .map_err(|e| crate::Error::ProofVerification(e.to_string())) |
| #[test] | ||
| fn test_nullifier_circuit_prove_verify() { | ||
| // Setup circuit | ||
| let (pk, vk) = NullifierCircuit::<Fr>::setup().expect("Circuit setup should succeed"); | ||
|
|
||
| let nullifier = Fr::from(42u64); | ||
| let path = vec![Fr::from(0u64); MERKLE_DEPTH]; | ||
| let indices = vec![false; MERKLE_DEPTH]; | ||
|
|
||
| // Compute root | ||
| let mut root = nullifier; | ||
| for i in 0..MERKLE_DEPTH { | ||
| let left = if indices[i] { path[i] } else { root }; | ||
| let right = if indices[i] { root } else { path[i] }; | ||
| root = left * left + right * right + left * right + Fr::from(1u64); | ||
| } | ||
|
|
||
| let circuit = NullifierCircuit { | ||
| nullifier: Some(nullifier), | ||
| set_root: Some(root), | ||
| is_member: Some(true), | ||
| path: Some(path), | ||
| indices: Some(indices), | ||
| }; | ||
|
|
||
| // Generate proof | ||
| let proof = circuit.prove(&pk).expect("Proof generation should succeed"); | ||
|
|
||
| // Verify proof using helper method | ||
| let public_inputs = circuit.public_inputs(); | ||
| assert!( | ||
| NullifierCircuit::verify(&vk, &proof, &public_inputs).expect("Verification should complete"), | ||
| "Proof verification should succeed" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The tests only verify the happy path (valid proof with correct public inputs). Consider adding negative test cases:
- Verification with incorrect public inputs (should return false)
- Verification with wrong verifying key (should fail)
- Tampered proof data (should fail)
Example:
#[test]
fn test_verify_with_wrong_public_inputs() {
let (pk, vk) = NullifierCircuit::<Fr>::setup().unwrap();
let circuit = /* ... valid circuit ... */;
let proof = circuit.prove(&pk).unwrap();
// Wrong public inputs
let wrong_inputs = vec![Fr::from(999u64), Fr::from(999u64), Fr::from(0u64)];
assert!(!NullifierCircuit::verify(&vk, &proof, &wrong_inputs).unwrap());
}| Ok(crate::Groth16Proof::new(proof)) | ||
| } | ||
|
|
||
| /// Verify a proof against public inputs |
There was a problem hiding this comment.
The verify() method documentation for NullifierCircuit is missing the public inputs specification. For consistency with StateCircuit and BattleCircuit, add documentation:
/// Verify a proof against public inputs
///
/// Public inputs should be in order:
/// 1. Nullifier
/// 2. Set root
/// 3. Is member (0 or 1)
pub fn verify(This helps users understand the expected input format.
| /// Verify a proof against public inputs | |
| /// Verify a proof against public inputs | |
| /// | |
| /// Public inputs should be in order: | |
| /// 1. Nullifier | |
| /// 2. Set root | |
| /// 3. Is member (0 or 1) |
| }; | ||
|
|
||
| Groth16::<Bn254>::circuit_specific_setup(circuit, rng) | ||
| .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e))) |
There was a problem hiding this comment.
The setup() method returns Error::ProofGeneration for circuit setup failures. Since this is specifically a setup error (not proof generation), consider using Error::Setup instead for semantic correctness:
.map_err(|e| crate::Error::Setup(format!("Circuit setup failed: {}", e)))This better matches the error type defined in lib.rs and makes debugging clearer.
| .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e))) | |
| .map_err(|e| crate::Error::Setup(format!("Circuit setup failed: {}", e))) |
| }; | ||
|
|
||
| Groth16::<Bn254>::circuit_specific_setup(circuit, rng) | ||
| .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e))) |
There was a problem hiding this comment.
The setup() method returns Error::ProofGeneration for circuit setup failures. Since this is specifically a setup error (not proof generation), consider using Error::Setup instead for semantic correctness:
.map_err(|e| crate::Error::Setup(format!("Circuit setup failed: {}", e)))This better matches the error type defined in lib.rs and makes debugging clearer.
| .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e))) | |
| .map_err(|e| crate::Error::Setup(format!("Circuit setup failed: {}", e))) |
|
|
||
| impl NullifierCircuit<Fr> { | ||
| /// Setup the circuit and generate proving/verifying keys | ||
| /// |
There was a problem hiding this comment.
[nitpick] The setup() method documentation for NullifierCircuit is missing the error handling description that appears in StateCircuit's setup(). For consistency, add:
/// Setup the circuit and generate proving/verifying keys
///
/// Returns an error if the circuit setup fails (e.g., due to constraint system issues).
///
/// **Note on RNG**: Uses `thread_rng()` which is cryptographically secure (ChaCha20-based).
/// For deterministic testing, consider using a seeded RNG from `ark_std::test_rng()`.
This maintains consistent documentation across all circuit implementations.
| /// | |
| /// | |
| /// Returns an error if the circuit setup fails (e.g., due to constraint system issues). | |
| /// |
| .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e))) | ||
| } | ||
|
|
||
| /// Generate a proof for this circuit instance |
There was a problem hiding this comment.
[nitpick] The prove() method lacks documentation. For API consistency with other methods, consider adding:
/// Generate a proof for this circuit instance
///
/// Returns an error if proof generation fails (e.g., due to unsatisfiable constraints).
pub fn prove(This helps users understand when and why proof generation might fail.
| /// Generate a proof for this circuit instance | |
| /// Generate a proof for this circuit instance | |
| /// | |
| /// Returns an error if proof generation fails (e.g., due to unsatisfiable constraints). |
| public_inputs: &[Fr], | ||
| ) -> crate::Result<bool> { | ||
| Groth16::<Bn254>::verify(vk, public_inputs, &proof.proof) | ||
| .map_err(|e| crate::Error::ProofVerification) |
There was a problem hiding this comment.
The error from Groth16::verify() is being discarded. This loses valuable debugging information about why verification failed. Consider preserving the error message like in the prove() method:
.map_err(|e| crate::Error::ProofVerification(e.to_string()))Note: This would require updating the Error::ProofVerification variant to accept a String parameter.
| .map_err(|e| crate::Error::ProofVerification) | |
| .map_err(|e| crate::Error::ProofVerification(e.to_string())) |
| pub fn public_inputs(&self) -> Vec<Fr> { | ||
| vec![ | ||
| self.nullifier.unwrap_or(Fr::from(0u64)), | ||
| self.set_root.unwrap_or(Fr::from(0u64)), | ||
| Fr::from(if self.is_member.unwrap_or(false) { 1u64 } else { 0u64 }), | ||
| ] |
There was a problem hiding this comment.
The public_inputs() method uses unwrap_or() with default values, which could silently hide missing data. If a field is None when it shouldn't be, this will use Fr::from(0u64) as the public input, potentially leading to incorrect verification. Consider either:
- Panicking when fields are None (they should be populated for proving/verification)
- Returning Result<Vec> to propagate errors
- Adding debug assertions to catch missing fields during development
Example:
pub fn public_inputs(&self) -> Vec<Fr> {
debug_assert!(self.nullifier.is_some(), "nullifier must be set");
vec![
self.nullifier.unwrap_or(Fr::from(0u64)),
// ...
]
}| pub fn public_inputs(&self) -> Vec<Fr> { | |
| vec![ | |
| self.nullifier.unwrap_or(Fr::from(0u64)), | |
| self.set_root.unwrap_or(Fr::from(0u64)), | |
| Fr::from(if self.is_member.unwrap_or(false) { 1u64 } else { 0u64 }), | |
| ] | |
| pub fn public_inputs(&self) -> crate::Result<Vec<Fr>> { | |
| let nullifier = self.nullifier.ok_or(crate::Error::MissingPublicInput("nullifier"))?; | |
| let set_root = self.set_root.ok_or(crate::Error::MissingPublicInput("set_root"))?; | |
| let is_member = self.is_member.ok_or(crate::Error::MissingPublicInput("is_member"))?; | |
| Ok(vec![ | |
| nullifier, | |
| set_root, | |
| Fr::from(if is_member { 1u64 } else { 0u64 }), | |
| ]) |
| pub fn public_inputs(&self) -> Vec<Fr> { | ||
| let mut inputs = Vec::new(); | ||
|
|
||
| // Add initial grid (flattened) | ||
| if let Some(ref grid) = self.initial_grid { | ||
| for row in grid { | ||
| for &cell in row { | ||
| inputs.push(Fr::from(cell as u64)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add final grid (flattened) | ||
| if let Some(ref grid) = self.final_grid { | ||
| for row in grid { | ||
| for &cell in row { | ||
| inputs.push(Fr::from(cell as u64)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add commitments and winner | ||
| if let Some(commitment_a) = self.commitment_a { | ||
| inputs.push(commitment_a); | ||
| } | ||
| if let Some(commitment_b) = self.commitment_b { | ||
| inputs.push(commitment_b); | ||
| } | ||
| if let Some(winner) = self.winner { | ||
| inputs.push(Fr::from(winner as u64)); | ||
| } | ||
|
|
||
| inputs |
There was a problem hiding this comment.
The public_inputs() method uses unwrap_or() with default values, which could silently hide missing data. If a field is None when it shouldn't be, this will use Fr::from(0u64) as the public input, potentially leading to incorrect verification. Consider either:
- Panicking when fields are None (they should be populated for proving/verification)
- Returning Result<Vec> to propagate errors
- Adding debug assertions to catch missing fields during development
Example:
pub fn public_inputs(&self) -> Vec<Fr> {
debug_assert!(self.initial_grid.is_some(), "initial_grid must be set");
let mut inputs = Vec::new();
if let Some(ref grid) = self.initial_grid {
// ...
}
// ...
}| pub fn public_inputs(&self) -> Vec<Fr> { | |
| let mut inputs = Vec::new(); | |
| // Add initial grid (flattened) | |
| if let Some(ref grid) = self.initial_grid { | |
| for row in grid { | |
| for &cell in row { | |
| inputs.push(Fr::from(cell as u64)); | |
| } | |
| } | |
| } | |
| // Add final grid (flattened) | |
| if let Some(ref grid) = self.final_grid { | |
| for row in grid { | |
| for &cell in row { | |
| inputs.push(Fr::from(cell as u64)); | |
| } | |
| } | |
| } | |
| // Add commitments and winner | |
| if let Some(commitment_a) = self.commitment_a { | |
| inputs.push(commitment_a); | |
| } | |
| if let Some(commitment_b) = self.commitment_b { | |
| inputs.push(commitment_b); | |
| } | |
| if let Some(winner) = self.winner { | |
| inputs.push(Fr::from(winner as u64)); | |
| } | |
| inputs | |
| pub fn public_inputs(&self) -> crate::Result<Vec<Fr>> { | |
| let mut inputs = Vec::new(); | |
| // Check required fields | |
| let initial_grid = self.initial_grid.as_ref().ok_or(crate::Error::MissingField("initial_grid"))?; | |
| let final_grid = self.final_grid.as_ref().ok_or(crate::Error::MissingField("final_grid"))?; | |
| let commitment_a = self.commitment_a.ok_or(crate::Error::MissingField("commitment_a"))?; | |
| let commitment_b = self.commitment_b.ok_or(crate::Error::MissingField("commitment_b"))?; | |
| let winner = self.winner.ok_or(crate::Error::MissingField("winner"))?; | |
| // Add initial grid (flattened) | |
| for row in initial_grid { | |
| for &cell in row { | |
| inputs.push(Fr::from(cell as u64)); | |
| } | |
| } | |
| // Add final grid (flattened) | |
| for row in final_grid { | |
| for &cell in row { | |
| inputs.push(Fr::from(cell as u64)); | |
| } | |
| } | |
| // Add commitments and winner | |
| inputs.push(commitment_a); | |
| inputs.push(commitment_b); | |
| inputs.push(Fr::from(winner as u64)); | |
| Ok(inputs) |
Replaces stubbed ZKP code with production Groth16 circuits. Battle outcomes and state transitions now require cryptographic proof verification, not boolean checks.
Architecture
Two-tier system:
BattleCircuit: Conway's Game of Life simulation (64×64 grid, 10 steps)StateCircuit: Merkle tree verification (depth 32)NullifierCircuit: Set membership for double-spend preventionSimpleBattleCircuit,SimpleStateCircuit)Implementation
battle_constraints.rs / state_constraints.rs (+348 lines)
setup(),prove(),verify(),public_inputs()to all circuitsthread_rng())lib.rs (+55 lines)
Usage
Testing
18 tests (17 passing, 1 performance test ignored). All consensus integration tests pass.
Production Notes
Current parameters (GRID_SIZE=64, BATTLE_STEPS=10) enable CI/CD feasibility. Mainnet deployment should:
poseidon_merkle.rs)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.