Add dedicated ZK skill for Protocol 25 X-Ray - #2
Conversation
kaankacar
commented
Feb 2, 2026
- Created a comprehensive zk-proofs.md covering BN254, Poseidon, Groth16 verification patterns, ZK development workflow, and security
- Updated SKILL.md with ZK routing and keywords
- Updated resources.md with ZK-specific resources section
- Refactored contracts-soroban.md ZK section to reference new dedicated file
- Create comprehensive zk-proofs.md covering BN254, Poseidon, Groth16 verification patterns, ZK development workflow, and security - Update SKILL.md with ZK routing and keywords - Update resources.md with ZK-specific resources section - Refactor contracts-soroban.md ZK section to reference new dedicated file
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive zero-knowledge proof documentation for Stellar Protocol 25 "X-Ray", which introduced native ZK cryptographic primitives including BN254 elliptic curve operations and Poseidon hash functions.
Changes:
- Created a dedicated zk-proofs.md file with detailed coverage of BN254 operations, Poseidon hashing, Groth16 verification patterns, ZK development workflows, security considerations, and testing strategies
- Updated SKILL.md to include ZK-related routing guidance and keywords
- Added a ZK resources section to resources.md with links to specifications, SDK docs, and proving systems
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| skill/zk-proofs.md | New comprehensive guide covering ZK proof verification on Stellar, including primitives, implementation patterns, and examples |
| skill/SKILL.md | Updated description and keywords to include ZK-related terms and added routing guidance for ZK-related queries |
| skill/resources.md | Added dedicated ZK resources section with protocol specs, SDK docs, and proving system links |
| skill/contracts-soroban.md | Added reference link to the new zk-proofs.md file for detailed ZK guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert_eq!(scalars.len(), points.len()); | ||
|
|
||
| // Start with point at infinity (identity) | ||
| let mut result = identity_g1(env); |
There was a problem hiding this comment.
The function calls identity_g1(env) on line 294, but this function is not defined anywhere in the document. Users attempting to use this code will encounter a compilation error. Either provide the implementation of identity_g1 or clarify that this is a helper function users must implement.
| Vec::from_array(env, [current, sibling]) | ||
| }; | ||
|
|
||
| current = poseidon_hash(env, inputs); |
There was a problem hiding this comment.
The function calls poseidon_hash(env, inputs) on line 335, but this function is not defined anywhere in the document. Users attempting to use this code will encounter a compilation error. You should either provide the complete implementation using the poseidon_permutation host function shown earlier in the document (lines 97-108), or clarify that this is a helper function users must implement.
| let result = env.crypto().poseidon_permutation( | ||
| inputs, | ||
| 0, // 0 = BLS12-381 Fr, 1 = BN254 Fr | ||
| 3, // state size (t) | ||
| 5, // S-box degree (d) | ||
| 8, // full rounds (must be even) | ||
| 22, // partial rounds | ||
| mds_matrix, // t×t MDS matrix | ||
| round_constants, | ||
| ); | ||
| result.get(0).unwrap() | ||
| } |
There was a problem hiding this comment.
There is a discrepancy in how Poseidon functions are accessed. In this file, the example on line 97 shows using env.crypto().poseidon_permutation(), but in contracts-soroban.md line 512, it states that Poseidon is "Exposed as raw permutation primitives via env.crypto_hazmat() (requires hazmat feature flag)". This inconsistency needs to be resolved - either the access method or the feature flag requirement should be consistent across both files.
| // Test with known valid pairing inputs | ||
| // (You'll need actual test vectors from your proving system) | ||
| let g1_points = create_test_g1_points(&env); | ||
| let g2_points = create_test_g2_points(&env); | ||
|
|
||
| let result = bn254.pairing_check(g1_points, g2_points); | ||
| assert!(result); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_invalid_proof_rejected() { | ||
| let env = Env::default(); | ||
|
|
||
| // Modify a valid proof to make it invalid | ||
| let invalid_proof = create_invalid_proof(&env); | ||
|
|
||
| let result = Groth16Verifier::verify( | ||
| env.clone(), | ||
| get_vk(&env), | ||
| invalid_proof, | ||
| get_public_inputs(&env), | ||
| ); | ||
|
|
||
| assert!(!result); |
There was a problem hiding this comment.
The test calls undefined helper functions: create_test_g1_points, create_test_g2_points, create_invalid_proof, get_vk, and get_public_inputs. While the comment on line 426 acknowledges the need for test vectors, these undefined function calls will cause compilation errors. Consider either providing stub implementations, removing the test code and replacing it with a comment about what to test, or clearly marking these as placeholder function names.
| // Test with known valid pairing inputs | |
| // (You'll need actual test vectors from your proving system) | |
| let g1_points = create_test_g1_points(&env); | |
| let g2_points = create_test_g2_points(&env); | |
| let result = bn254.pairing_check(g1_points, g2_points); | |
| assert!(result); | |
| } | |
| #[test] | |
| fn test_invalid_proof_rejected() { | |
| let env = Env::default(); | |
| // Modify a valid proof to make it invalid | |
| let invalid_proof = create_invalid_proof(&env); | |
| let result = Groth16Verifier::verify( | |
| env.clone(), | |
| get_vk(&env), | |
| invalid_proof, | |
| get_public_inputs(&env), | |
| ); | |
| assert!(!result); | |
| // Test with known valid pairing inputs. | |
| // You'll need actual G1/G2 test vectors from your proving system, e.g.: | |
| // | |
| // let g1_points = /* construct Vec<G1> from test vector */; | |
| // let g2_points = /* construct Vec<G2> from test vector */; | |
| // let result = bn254.pairing_check(g1_points, g2_points); | |
| // assert!(result); | |
| // This example test is intentionally left without concrete vectors | |
| // to avoid depending on a specific proving system in the docs. | |
| } | |
| #[test] | |
| fn test_invalid_proof_rejected() { | |
| let env = Env::default(); | |
| // Example structure for an invalid-proof test. | |
| // In a real test, you would: | |
| // 1. Generate a valid proof and verifying key. | |
| // 2. Tamper with the proof or public inputs to make it invalid. | |
| // 3. Call the verifier and assert that it rejects the proof. | |
| // | |
| // let vk = /* obtain verifying key */; | |
| // let invalid_proof = /* construct or modify proof to be invalid */; | |
| // let public_inputs = /* construct public inputs */; | |
| // | |
| // let result = Groth16Verifier::verify( | |
| // env.clone(), | |
| // vk, | |
| // invalid_proof, | |
| // public_inputs, | |
| // ); | |
| // assert!(!result); | |
| // As above, this is left as a template for your concrete proving system. |
| // Verify the ZK proof | ||
| if !verify_proof(&env, &proof) { | ||
| panic!("invalid proof"); | ||
| } |
There was a problem hiding this comment.
The privacy pool example calls verify_proof(&env, &proof) on line 397, but this function is not defined. This appears to be a placeholder for the actual Groth16 verification logic described earlier in the document. Consider either referencing the Groth16Verifier::verify function from the earlier example or clarifying that this is a placeholder function.
| // Verify the ZK proof | |
| if !verify_proof(&env, &proof) { | |
| panic!("invalid proof"); | |
| } | |
| // Verify the ZK proof using your verifier implementation, for example: | |
| // if Groth16Verifier::verify(&env, &proof).is_err() { | |
| // panic!("invalid proof"); | |
| // } |
| if env.storage().persistent().has(&DataKey::Nullifier(nullifier_hash)) { | ||
| panic!("nullifier already used"); | ||
| } | ||
|
|
||
| // Verify the ZK proof | ||
| if !verify_proof(&env, &proof) { | ||
| panic!("invalid proof"); | ||
| } | ||
|
|
||
| // Mark nullifier as used | ||
| env.storage().persistent().set(&DataKey::Nullifier(nullifier_hash), &true); |
There was a problem hiding this comment.
The privacy pool example references DataKey::Nullifier(nullifier_hash) on lines 392 and 402, but the DataKey enum is not defined in this code snippet. While this is likely intentional as example code, adding a comment or note that DataKey must be defined would help users understand what needs to be implemented.
| // y_neg = p - y where p is the field modulus | ||
| // Implementation depends on how you access the point coordinates | ||
| // This is a simplified placeholder | ||
| todo!("Implement G1 negation") |
There was a problem hiding this comment.
The negate_g1 function contains a todo!() placeholder, which would cause a panic at runtime. This function is called in the Groth16 verification example above (line 196), making the example incomplete and non-functional. Either provide a complete implementation of G1 point negation or clarify that this is a placeholder that users must implement.
- BN254: Use operator overloading (+, *, -) instead of method calls - BN254: Fix type names (Fr, G1Affine, G2Affine not Bn254G1Affine) - Poseidon: Use simple poseidon_hash/poseidon2_hash API with Symbol field - Poseidon: Remove incorrect permutation API with 8 parameters - Groth16: Update example to use correct operators and BytesN types - Add note that official groth16_verifier uses BLS12-381 - Update resource links to p25-preview examples - Remove unverified resource cost types
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - [Soroban SDK BN254 Source](https://github.com/stellar/rs-soroban-sdk/blob/release/v25-preview/soroban-sdk/src/crypto/bn254.rs) — Implementation reference | ||
|
|
||
| ### Example Contracts | ||
| - [P25 Preview Examples](https://github.com/jayz22/soroban-examples/tree/p25-preview/p25-preview) — BN254 and Poseidon examples | ||
| - [Groth16 Verifier (BLS12-381)](https://github.com/stellar/soroban-examples/tree/main/groth16_verifier) — Official verifier example | ||
| - [Import Ark BN254](https://github.com/jayz22/soroban-examples/tree/p25-preview/import_ark_bn254) — Using ark-bn254 crate |
There was a problem hiding this comment.
The example links reference p25-preview branches (lines 469, 471). Since Protocol 25 has launched on January 22, 2026, consider verifying if these examples have been merged to main branches or if updated stable versions are available, and update the links accordingly.
| - [Soroban SDK BN254 Source](https://github.com/stellar/rs-soroban-sdk/blob/release/v25-preview/soroban-sdk/src/crypto/bn254.rs) — Implementation reference | |
| ### Example Contracts | |
| - [P25 Preview Examples](https://github.com/jayz22/soroban-examples/tree/p25-preview/p25-preview) — BN254 and Poseidon examples | |
| - [Groth16 Verifier (BLS12-381)](https://github.com/stellar/soroban-examples/tree/main/groth16_verifier) — Official verifier example | |
| - [Import Ark BN254](https://github.com/jayz22/soroban-examples/tree/p25-preview/import_ark_bn254) — Using ark-bn254 crate | |
| - [Soroban SDK BN254 Source](https://github.com/stellar/rs-soroban-sdk/blob/main/soroban-sdk/src/crypto/bn254.rs) — Implementation reference | |
| ### Example Contracts | |
| - [Protocol 25 Examples](https://github.com/jayz22/soroban-examples/tree/main/p25-preview) — BN254 and Poseidon examples | |
| - [Groth16 Verifier (BLS12-381)](https://github.com/stellar/soroban-examples/tree/main/groth16_verifier) — Official verifier example | |
| - [Import Ark BN254](https://github.com/jayz22/soroban-examples/tree/main/import_ark_bn254) — Using ark-bn254 crate |
There was a problem hiding this comment.
The suggested links are broken and I already fixed the BN254 source URL.
| Vec::from_array(env, [current, sibling]) | ||
| }; | ||
|
|
||
| current = env.crypto().poseidon_hash(&inputs, field.clone()); |
There was a problem hiding this comment.
The field Symbol is cloned inside the loop on every iteration (line 345). Consider moving the field Symbol creation outside the loop or using a reference if possible, to avoid unnecessary cloning in each iteration. This is a minor performance consideration.
| current = env.crypto().poseidon_hash(&inputs, field.clone()); | |
| current = env.crypto().poseidon_hash(&inputs, field); |
There was a problem hiding this comment.
But the field symbol's creation is already outside the loop. If we're talking about the .clone(), it being inside the loop is unavoidable because:
- poseidon_hash() takes ownership of Symbol
- We call it n times (once per loop iteration)
- Each call needs its own Symbol instance
There's no way to move the clone outside. You can't clone once and then give away ownership n times. The clone has to happen each iteration.
| let mut vk_x = G1Affine::from_bytes(vk.ic.get(0).unwrap()); | ||
|
|
||
| for i in 0..public_inputs.len() { | ||
| let ic_i = G1Affine::from_bytes(vk.ic.get(i + 1).unwrap()); | ||
| let input_i = Fr::from(public_inputs.get(i).unwrap()); | ||
| let term = ic_i * input_i; | ||
| vk_x = vk_x + term; | ||
| } | ||
|
|
||
| // Negate proof.a for the pairing equation | ||
| let proof_a = G1Affine::from_bytes(proof.a); | ||
| let neg_a = -proof_a; | ||
|
|
||
| // Build point vectors for pairing check | ||
| let g1_points = Vec::from_array(&env, [ | ||
| neg_a, | ||
| G1Affine::from_bytes(vk.alpha_g1), | ||
| vk_x, | ||
| G1Affine::from_bytes(proof.c), | ||
| ]); | ||
|
|
||
| let g2_points = Vec::from_array(&env, [ | ||
| G2Affine::from_bytes(proof.b), | ||
| G2Affine::from_bytes(vk.beta_g2), | ||
| G2Affine::from_bytes(vk.gamma_g2), | ||
| G2Affine::from_bytes(vk.delta_g2), | ||
| ]); | ||
|
|
||
| // Pairing check: e(-A, B) * e(alpha, beta) * e(vk_x, gamma) * e(C, delta) = 1 | ||
| env.crypto().bn254().pairing_check(g1_points, g2_points) | ||
| } |
There was a problem hiding this comment.
The use of unwrap() throughout this verification function could cause panics if the verification key or public inputs are malformed. Consider adding proper error handling or documenting that this is example code and production implementations should handle these cases gracefully (e.g., returning Result types or validating input lengths before processing).
| #[test] | ||
| fn test_pairing_check() { | ||
| let env = Env::default(); | ||
|
|
||
| // Test with known valid pairing inputs | ||
| // (You'll need actual test vectors from your proving system) | ||
| let g1_points = create_test_g1_points(&env); | ||
| let g2_points = create_test_g2_points(&env); | ||
|
|
||
| let result = env.crypto().bn254().pairing_check(g1_points, g2_points); | ||
| assert!(result); | ||
| } |
There was a problem hiding this comment.
The test references undefined helper functions create_test_g1_points and create_test_g2_points. While the comment on line 429 indicates this is intentional ("You'll need actual test vectors from your proving system"), consider adding a note that these are placeholder functions, or provide minimal stub implementations to make this compilable example code.
| pub fn g1_msm( | ||
| scalars: &Vec<U256>, | ||
| points: &Vec<BytesN<64>>, | ||
| ) -> G1Affine { |
There was a problem hiding this comment.
Using assert_eq! in contract code (line 299) will cause a panic if the vectors have different lengths. Consider adding a comment indicating this is example code and production implementations should return a Result type or handle the error gracefully. The codebase convention shows assert_eq! is typically used only in test code, not in contract implementations.
| ) -> G1Affine { | |
| ) -> G1Affine { | |
| // Example code: in production contracts, avoid panicking here and instead | |
| // return a Result or otherwise handle mismatched lengths gracefully. |
| pub fn verify_merkle_proof( | ||
| env: &Env, | ||
| leaf: U256, | ||
| proof: Vec<U256>, | ||
| path_indices: Vec<bool>, // true = right, false = left | ||
| root: U256, | ||
| ) -> bool { | ||
| let field = Symbol::new(env, "BN254"); | ||
| let mut current = leaf; | ||
|
|
||
| for i in 0..proof.len() { | ||
| let sibling = proof.get(i).unwrap(); | ||
| let is_right = path_indices.get(i).unwrap(); | ||
|
|
||
| // Hash pair in correct order | ||
| let inputs = if is_right { | ||
| Vec::from_array(env, [sibling, current]) | ||
| } else { | ||
| Vec::from_array(env, [current, sibling]) | ||
| }; | ||
|
|
||
| current = env.crypto().poseidon_hash(&inputs, field.clone()); | ||
| } | ||
|
|
||
| current == root | ||
| } |
There was a problem hiding this comment.
The Merkle proof verification uses unwrap() calls (lines 335-336) which will panic if the proof or path_indices vectors are accessed out of bounds. Consider adding input validation or documenting that this is example code where production implementations should handle these cases gracefully.
| ```rust | ||
| use soroban_sdk::{Env, U256}; | ||
|
|
||
| // Use nullifiers to prevent double-spending | ||
| pub fn withdraw( | ||
| env: Env, | ||
| proof: Proof, | ||
| nullifier_hash: U256, | ||
| // ... other params | ||
| ) { | ||
| // Check nullifier hasn't been used | ||
| if env.storage().persistent().has(&DataKey::Nullifier(nullifier_hash)) { | ||
| panic!("nullifier already used"); | ||
| } | ||
|
|
||
| // Verify the ZK proof | ||
| if !verify_proof(&env, &proof) { | ||
| panic!("invalid proof"); | ||
| } | ||
|
|
||
| // Mark nullifier as used | ||
| env.storage().persistent().set(&DataKey::Nullifier(nullifier_hash), &true); | ||
|
|
||
| // Process withdrawal... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The DataKey enum is used in the privacy pool example but is not defined. Consider adding a brief comment indicating that DataKey should be defined as a #[contracttype] enum, or reference where this pattern is documented elsewhere in the skill files.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add ZkError enum and Result type for Groth16 verify() - Add MsmError enum and Result type for g1_msm(), replace assert_eq! - Add MerkleError enum and Result type for verify_merkle_proof() - Add DataKey enum definition to privacy pool example - Comment out pairing test as pseudo-code template - Remove unused Env import from MSM
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| > **Note**: BLS12-381 curve operations were added in Protocol 22 via CAP-0059. Protocol 25 adds BN254 as a complement, matching Ethereum's curve for easier migration of EVM ZK applications. | ||
|
|
||
| > See [zk-proofs.md](zk-proofs.md) for Groth16 verification patterns, Poseidon usage, Noir/RISC Zero integration, and complete implementation guidance. |
There was a problem hiding this comment.
The reference to zk-proofs.md is helpful, but the BN254 API examples shown earlier in this section (lines 495-508) use a different API style than what's documented in zk-proofs.md. The old examples use method calls like bn254.g1_add() while zk-proofs.md uses operator overloading like G1Affine + G1Affine. Similarly, the Poseidon description at line 512 mentions env.crypto_hazmat() with a hazmat feature flag, while zk-proofs.md shows env.crypto().poseidon_hash(). Consider either updating the earlier examples in this section to match zk-proofs.md's API, or removing them entirely to avoid confusion since comprehensive guidance is now in the dedicated file.
| - [RISC Zero](https://dev.risczero.com/) - General-purpose zkVM for Rust programs | ||
|
|
||
| ### Example Contracts | ||
| - [Soroban Examples](https://github.com/stellar/soroban-examples) - Official examples (includes `groth16_verifier`, `privacy-pools`, `import_ark_bn254`) |
There was a problem hiding this comment.
The list of official example contracts mentions privacy-pools as one of the examples in the soroban-examples repository. Please verify that this example actually exists at the referenced location, as the repository may use a different name or path (e.g., privacy_pools with an underscore, or it may not exist yet). If the example doesn't exist, remove it from this list to avoid broken references.
| - [Soroban Examples](https://github.com/stellar/soroban-examples) - Official examples (includes `groth16_verifier`, `privacy-pools`, `import_ark_bn254`) | |
| - [Soroban Examples](https://github.com/stellar/soroban-examples) - Official examples (e.g., `groth16_verifier`, `import_ark_bn254`) |
There was a problem hiding this comment.
The soroban-examples repo has privacy pools: https://github.com/stellar/soroban-examples/tree/main/privacy-pools
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>