feat: add pk_commitment for greco#1083
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis PR adds public key commitment validation to the Greco protocol by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
0xjei
force-pushed
the
feat/greco-commit
branch
from
c9cced8 to
30e53e5
Compare
|
I am currently working on aligning Since this might introduce some minor breaking change in generator / enclave, I am doing some tests before hand. I will expect this PR to be key to unlock all the errors we might have once we merge the PR above to the main branch of fhe.rs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/CRISP/circuits/src/main.nr (1)
29-29: Consider making pk_commitment public.The
pk_commitmentfield has a TODO comment suggesting it should be public. Since this is a commitment value that likely needs to be verified externally, making it public would allow the verifier to check it.Apply this diff if
pk_commitmentshould be public for verification:- pk_commitment: Field, // todo: make this pub + pk_commitment: pub Field,Alternatively, if this TODO is tracked elsewhere and will be addressed later, this can be deferred.
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
94-112: RedundantGrecoBounds::computecall.
GrecoBounds::computeis called twice with identical arguments (lines 94 and 111). The first call extracts onlybit_pk, discardingcrypto_params, while the second call uses both return values. Consider computing bounds once and reusing the result.- let (_, bounds) = GrecoBounds::compute(&self.bfv_params, 0)?; - - let bit_pk = shared::template::calculate_bit_width(&bounds.pk_bounds[0].to_string())?; + let (crypto_params, bounds) = GrecoBounds::compute(&self.bfv_params, 0) + .with_context(|| "Failed to compute bounds")?; + + let bit_pk = shared::template::calculate_bit_width(&bounds.pk_bounds[0].to_string())?; // Compute the vectors of the GRECO inputs. let greco_vectors = GrecoVectors::compute( &pt, &u_rns, &e0_rns, &e1_rns, &ct, &pk, &self.bfv_params, bit_pk, ) .with_context(|| "Failed to compute vectors")?; - let (crypto_params, bounds) = GrecoBounds::compute(&self.bfv_params, 0) - .with_context(|| "Failed to compute bounds")?; + // crypto_params and bounds already computed above
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamltemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
circuits/crates/libs/greco/src/lib.nr(8 hunks)crates/bfv-helpers/src/client.rs(3 hunks)crates/bfv-helpers/src/utils/greco.rs(2 hunks)crates/tests/tests/integration.rs(2 hunks)crates/trbfv/src/helpers.rs(1 hunks)crates/trbfv/tests/integration.rs(2 hunks)examples/CRISP/circuits/src/main.nr(2 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(1 hunks)examples/CRISP/crates/zk-inputs/src/serialization.rs(5 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol(2 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/tests/tests/integration.rscrates/trbfv/tests/integration.rs
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
crates/tests/tests/integration.rscrates/trbfv/tests/integration.rs
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
crates/tests/tests/integration.rscrates/trbfv/tests/integration.rs
📚 Learning: 2025-09-22T15:08:29.814Z
Learnt from: ozgurarmanc
Repo: gnosisguild/enclave PR: 734
File: packages/circuits/crates/libs/polynomial/src/lib.nr:140-155
Timestamp: 2025-09-22T15:08:29.814Z
Learning: Greco (packages/circuits/crates/libs/greco/src/lib.nr) performs range_check_1bound/2bounds on all polynomials (u, e0/e1, k1, pk*, r*, p*) before serialization; packer/flatten rely on these bounds, so per-limb asserts inside packer are unnecessary in this crate’s flow.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rscircuits/crates/libs/greco/src/lib.nrcrates/bfv-helpers/src/client.rscrates/bfv-helpers/src/utils/greco.rsexamples/CRISP/circuits/src/main.nr
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/bfv-helpers/src/client.rs
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
crates/trbfv/tests/integration.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/trbfv/tests/integration.rs
🧬 Code graph analysis (3)
crates/tests/tests/integration.rs (1)
crates/trbfv/src/helpers.rs (1)
calculate_error_size(47-56)
crates/trbfv/src/helpers.rs (1)
crates/trbfv/src/trbfv_config.rs (1)
params(34-36)
crates/trbfv/tests/integration.rs (3)
crates/test-helpers/src/lib.rs (2)
create_seed_from_u64(40-42)new(168-174)crates/crypto/src/cipher.rs (1)
from_password(123-125)crates/trbfv/src/helpers.rs (1)
calculate_error_size(47-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: crisp_unit
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: test_contracts
🔇 Additional comments (21)
crates/trbfv/src/helpers.rs (1)
47-56: LGTM! Breaking API change for security parameter.The addition of the
lambdaparameter (security parameter for smudging bound calculation) is well-documented and properly threaded through toSmudgingBoundCalculatorConfig::new. The change is intentional per the PR objectives.examples/CRISP/packages/crisp-sdk/src/types.ts (1)
134-134: LGTM! Consistent type addition.The
pk_commitment: stringfield is correctly added to theCircuitInputstype in the Greco section, aligning with the serialization and circuit changes.examples/CRISP/crates/zk-inputs/src/serialization.rs (5)
42-42: LGTM! Field addition to ZKInputs.The
pk_commitment: Stringfield is correctly added to theZKInputsstruct.
259-259: LGTM! Field population.The
pk_commitmentfield is correctly populated by convertingvectors_standard.pk_commitmentto a string, following the same pattern as other fields.
344-353: LGTM! Test data updated with pk_commitment.The mock test data correctly includes
pk_commitment: BigInt::from(45)in theGrecoVectorsstruct, aligning with the serialization changes.
397-397: LGTM! Test assertion for pk_commitment.The test correctly verifies that
pk_commitmentis populated by checking its length is greater than 0.
436-436: LGTM! Test assertion for JSON serialization.The test correctly verifies that the
pk_commitmentfield is present in the serialized JSON output.crates/tests/tests/integration.rs (2)
150-154: LGTM! Insecure test parameter clearly documented.The warning comment clearly states that
lambda = 2is an INSECURE parameter for testing purposes only. Production use requireslambda = 80for adequate security.
157-162: LGTM! Lambda parameter correctly threaded.The
lambdaparameter is correctly passed tocalculate_error_size, aligning with the updated function signature incrates/trbfv/src/helpers.rs.examples/CRISP/circuits/src/main.nr (1)
77-94: LGTM! pk_commitment correctly threaded to Greco::new.The
pk_commitmentparameter is correctly passed to theGreco::newconstructor as the first argument afterparams, aligning with the updated Greco implementation.crates/bfv-helpers/src/utils/greco.rs (1)
224-231: LGTM! Test helper correctly updated for bit width parameter.The test setup properly computes Greco bounds, derives the bit width, and passes it to
GrecoVectors::compute. The pattern of passing0as the second parameter toGrecoBounds::computeis consistent across all production usages inclient.rsand example code, confirming it is the intended value.examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (1)
11-129: Verification keys updated for circuit changes.The VK_HASH and G1Point coordinates have been regenerated to reflect the circuit modifications, including the addition of
pk_commitmentto the Greco section. These values are circuit-specific artifacts derived from the compiled Noir circuit and must match the updated circuit structure. The changes appear correct based on the circuit dependencies and structure.crates/bfv-helpers/src/client.rs (1)
107-121: LGTM! Bit width derivation for pk bounds is correct.The code properly computes Greco bounds and derives the bit width from
bounds.pk_bounds[0], following the established pattern across the codebase. Error handling is properly maintained. The first return value fromGrecoBounds::computeis intentionally discarded sinceGrecoVectors::computerequires only the bounds, not the cryptographic parameters.crates/trbfv/tests/integration.rs (3)
64-77: Security warning comment is appropriate; lambda=2 is suitable for tests only.The explicit warning about the insecure lambda parameter is well documented. The comment correctly notes that production should use lambda=80. This approach aligns with standard practice of using reduced security parameters to speed up test execution.
71-79: LGTM!The
Cipher::from_passwordusage with a hardcoded password is acceptable for test purposes. TheTrBFVConfig::newcall correctly casts threshold parameters tou64, andcalculate_error_sizeproperly includes the newlambdaparameter matching the updated function signature.
86-93: LGTM!The
generate_shares_hash_mapcall correctly passesesi_per_ct as u64and includes the newcipherparameter, aligning with the updated function signature.circuits/crates/libs/greco/src/lib.nr (5)
183-201: LGTM! pk_commitment field addition is well integrated.The
pk_commitment: Fieldfield is correctly added to theGrecostruct, and the documentation at line 175 appropriately describes its purpose as a public key commitment.
229-265: LGTM!The constructor correctly accepts
pk_commitmentas a parameter and initializes the field. The parameter ordering (afterparams) and documentation are consistent.
276-298: LGTM!The
gammas_payloadfunction correctly includespk_commitmentas the first element in the payload before flattening ciphertext polynomials. This ensures the Fiat-Shamir challenge is bound to the public key commitment, maintaining the cryptographic integrity of the proof.
300-315: LGTM!The
commitment_payloadfunction correctly serializespk0isandpk1isusing theBIT_PKbit-width parameter, consistent with the range checks applied to these polynomials incheck_range_bounds. The documentation clearly describes its purpose.
440-473: Commitment verification logic is correctly implemented.The
generate_challengefunction now performs two-phase sponge operations:
- First sponge absorbs public key polynomials via
commitment_payload()and verifies the squeezed commitment matchespk_commitment(line 460)- Second sponge generates gammas from
gammas_payload()for the Schwartz-Zippel polynomial identity testingThis ensures the prover is bound to the committed public key, preventing substitution attacks.
2 participants
This PR adds a
pk_commitmentto theGrecocircuit.PLEASE NOTE THAT: we are defaulting to dummy parameters branches in
fhe.rsandzkfhe-generator. We need to understand what to do and how to progress before merging this.cc @ryardley @cedoor @ctrlc03 @hmzakhalid
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.