feat: crisp use param set 512_10_1#1009
Conversation
WalkthroughRenamed BFV parameter enum to Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ZK as ZKInputs
participant Helpers as bfv_helpers
participant Set as BfvParamSet
Note over Caller,ZK: Constructing ZK inputs (two paths)
Caller->>ZK: new(degree, pt_mod, moduli, error_var?)
ZK->>Helpers: build_bfv_params_arc(degree,...)
Helpers-->>ZK: Arc<BfvParameters>
ZK-->>Caller: Self
Note over Caller,ZK: From explicit param set
Caller->>ZK: from_set(set: BfvParamSet)
ZK->>Set: set.build_arc()
Set-->>ZK: Arc<BfvParameters>
ZK-->>Caller: Self
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-05-07T15:18:20.056ZApplied to files:
📚 Learning: 2025-11-08T01:31:47.505ZApplied to files:
📚 Learning: 2024-10-22T02:10:34.864ZApplied to files:
📚 Learning: 2024-11-05T06:48:58.177ZApplied to files:
🧬 Code graph analysis (1)examples/CRISP/server/src/server/routes/rounds.rs (2)
⏰ 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). (11)
🔇 Additional comments (2)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/bfv-helpers/src/lib.rs(2 hunks)examples/CRISP/crates/zk-inputs-wasm/src/lib.rs(1 hunks)examples/CRISP/crates/zk-inputs/Cargo.toml(1 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(13 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/crates/zk-inputs/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/crates/zk-inputs/Cargo.toml
📚 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:
examples/CRISP/crates/zk-inputs/src/lib.rscrates/bfv-helpers/src/lib.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rs
📚 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:
examples/CRISP/crates/zk-inputs/src/lib.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:
examples/CRISP/crates/zk-inputs/src/lib.rs
🧬 Code graph analysis (3)
examples/CRISP/crates/zk-inputs/src/lib.rs (3)
crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(288-310)examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
new(30-41)with_defaults(45-48)examples/CRISP/crates/zk-inputs/src/ciphertext_addition.rs (1)
new(41-50)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
examples/CRISP/crates/zk-inputs/src/lib.rs (2)
new(42-50)with_defaults(66-68)
crates/bfv-helpers/src/lib.rs (3)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
new(30-41)examples/CRISP/crates/zk-inputs/src/lib.rs (1)
new(42-50)crates/fhe/src/fhe.rs (1)
new(49-51)
⏰ 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). (1)
- GitHub Check: Build & Push Image
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
templates/default/program/src/lib.rs (1)
30-47: Tests: consider building BFV params directly fromBfvParamSetThe test logic is fine, but now that you’re using
BfvParamSetyou don’t really need to thread its fields throughbuild_bfv_params_arcmanually.For example, you could simplify to something like:
let params_set: BfvParamSet = BfvParamSets::InsecureSet2048_1032193_1.into(); let params = params_set.build_arc(); // or build_bfv_params_from_set_arc(params_set)This keeps the test in sync with the
BfvParamSetAPI and avoids repeating the struct’s field list.crates/sdk/Cargo.toml (1)
10-19: Feature-gated helpers look good; consider documenting feature usage for downstream cratesMaking
e3-evm-helpers,e3-bfv-helpers, ande3-indexeroptional and wiring them behindevm,bfv, andindexerfeatures with afulldefault is a clean way to trim transitive dependencies when needed. Just ensure crates that rely one3_sdk::bfv_helpers/evm_helpers/indexerexplicitly enable the corresponding features (and that this is mentioned in SDK docs or README) so consumers who disable default features don’t get surprising missing symbols.examples/CRISP/crates/zk-inputs/src/lib.rs (3)
38-46: Constructors are consistent with the new BFV helper ergonomicsThe trio of constructors is nicely layered:
new(...) -> Selffor raw (degree, plaintext, moduli, variance) inputs viabuild_bfv_params_arc.from_set(BfvParamSet)defers toBfvParamSet::build_arc, aligning with the new helper API.with_defaults()now pulls fromget_default_paramset(), so CRISP’s default tracks the centralizedBfvParamSetsdefinition.This removes Result-based plumbing here and leaves parameter validation/panics to the helpers, which is acceptable for test/demo-only defaults. No functional issues spotted.
You could consider making
newandfrom_setreturnResult<Self>in the future if you ever want to surface parameter-construction errors instead of panicking, but that’s not required for this PR.Also applies to: 48-53, 62-64
198-205: Avoid duplicating insecure param literals; prefer using BfvParamSets in testsBoth tests that exercise
from_setconstructBfvParamSetwith hard-coded2048 / 1032193 / [0x3FFFFFFF000001]. Since these values already live inBfvParamSets::InsecureSet2048_1032193_1, you can reduce drift risk and keep tests aligned with the enum by reusing it:- let generator = ZKInputsGenerator::from_set(BfvParamSet { - degree: 2048, - plaintext_modulus: 1032193, - moduli: &[0x3FFFFFFF000001], - error1_variance: None, - }); + let generator = + ZKInputsGenerator::from_set(BfvParamSets::InsecureSet2048_1032193_1.into());Same pattern applies in
test_get_bfv_params.Also applies to: 245-250
180-196: Tests exercise the new default and constructor paths; minor naming/comment nitsThe updated tests now:
- Use
with_defaults()consistently for the default CRISP param set (512-degree).- Use
from_setfor non-default/insecure 2048-parameter tests.- Drive end-to-end flows (keygen, encrypt, generate_inputs, JSON structure) under the new wiring, which is valuable coverage.
Two small nits that you may want to adjust later:
test_cryptographic_properties’s first assertion block is labeled “different votes produce different ciphertexts” but both encryptions use the same vote; it’s actually testing that the same vote produces different ciphertexts due to randomness (which is a good property). Either change the label or use genuinely different vote vectors there.- Several tests reuse
create_vote_vector()independent of the underlying degree; that’s fine semantically (shorter messages are valid), but if you ever parametrize tests over multiple sets it might be clearer to derive the vote length from the selectedBfvParamSet.Functionally everything looks sound.
Also applies to: 223-241, 260-280, 285-299, 304-320, 324-353, 357-383
crates/bfv-helpers/src/lib.rs (2)
99-142: From mapping is coherent with documented parametersThe
From<BfvParamSets> for BfvParamSetimplementation correctly:
- Sets
degree,plaintext_modulus, andmodulito the documented values for each variant.- Wires
InsecureSet512_10_1to degree 512, plaintext modulus 10, and the two expected moduli witherror1_variance: Some("3").- Wires the TRBFV production set to the long decimal variance string.
This gives a single canonical mapping for all param sets and plays nicely with the new
BfvParamSet::build/build_archelpers.If you want to avoid repeating these literals in multiple places over time, consider centralizing them (e.g., associated consts on
BfvParamSets) and constructing theBfvParamSetfrom those, but that’s optional.
161-171: BfvParamSet::build / build_arc ergonomics are good; small reuse opportunityAdding:
build(self) -> BfvParametersdelegating tobuild_bfv_params_from_set, andbuild_arc(self) -> Arc<BfvParameters>nicely complements the existing free functions and simplifies call sites like CRISP’s
from_set. One small redundancy:pub fn build_arc(self) -> Arc<BfvParameters> { Arc::new(self.build()) }could instead call the existing helper:
pub fn build_arc(self) -> Arc<BfvParameters> { build_bfv_params_from_set_arc(self) }to mirror
build’s delegation and reuse theBfvParametersBuilder::build_arcpath directly. Behavior is equivalent; this is just a minor refactor.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/CRISP/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.github/workflows/ci.yml(1 hunks)crates/bfv-helpers/src/client.rs(5 hunks)crates/bfv-helpers/src/lib.rs(9 hunks)crates/evm/src/helpers.rs(3 hunks)crates/sdk/Cargo.toml(1 hunks)crates/sdk/src/lib.rs(1 hunks)crates/test-helpers/src/bin/fake_encrypt.rs(2 hunks)crates/test-helpers/src/lib.rs(2 hunks)crates/wasm/src/lib.rs(3 hunks)examples/CRISP/Cargo.toml(3 hunks)examples/CRISP/circuits/src/main.nr(3 hunks)examples/CRISP/crates/crisp-constants/Cargo.toml(1 hunks)examples/CRISP/crates/crisp-constants/src/lib.rs(1 hunks)examples/CRISP/crates/zk-inputs-wasm/src/lib.rs(2 hunks)examples/CRISP/crates/zk-inputs/Cargo.toml(1 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(11 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol(2 hunks)examples/CRISP/scripts/dev_cipher.sh(1 hunks)examples/CRISP/server/Cargo.toml(1 hunks)examples/CRISP/server/src/cli/commands.rs(2 hunks)examples/CRISP/server/src/server/routes/rounds.rs(2 hunks)examples/CRISP/test/crisp.spec.ts(5 hunks)templates/default/program/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/CRISP/Cargo.toml
- examples/CRISP/crates/zk-inputs-wasm/src/lib.rs
- examples/CRISP/crates/zk-inputs/Cargo.toml
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
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.
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.
📚 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/test-helpers/src/lib.rscrates/bfv-helpers/src/client.rstemplates/default/program/src/lib.rscrates/sdk/src/lib.rsexamples/CRISP/crates/crisp-constants/src/lib.rsexamples/CRISP/server/src/cli/commands.rscrates/test-helpers/src/bin/fake_encrypt.rsexamples/CRISP/server/src/server/routes/rounds.rsexamples/CRISP/crates/zk-inputs/src/lib.rscrates/bfv-helpers/src/lib.rscrates/wasm/src/lib.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/test-helpers/src/lib.rstemplates/default/program/src/lib.rsexamples/CRISP/scripts/dev_cipher.shexamples/CRISP/server/src/cli/commands.rscrates/test-helpers/src/bin/fake_encrypt.rsexamples/CRISP/server/src/server/routes/rounds.rscrates/wasm/src/lib.rs
📚 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/test-helpers/src/lib.rsexamples/CRISP/server/src/cli/commands.rscrates/test-helpers/src/bin/fake_encrypt.rsexamples/CRISP/server/src/server/routes/rounds.rscrates/evm/src/helpers.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/test-helpers/src/lib.rstemplates/default/program/src/lib.rsexamples/CRISP/crates/zk-inputs/src/lib.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/sdk/src/lib.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/crates/crisp-constants/Cargo.tomlcrates/sdk/Cargo.toml
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/crates/crisp-constants/Cargo.tomlcrates/sdk/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/server/Cargo.toml
📚 Learning: 2024-10-23T01:59:27.215Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: tests/basic_integration/test.sh:21-21
Timestamp: 2024-10-23T01:59:27.215Z
Learning: In `tests/basic_integration/test.sh`, the hardcoded `CIPHERNODE_SECRET` is acceptable for testing purposes and does not need to be changed.
Applied to files:
examples/CRISP/scripts/dev_cipher.sh
📚 Learning: 2024-09-26T04:12:09.345Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:103-114
Timestamp: 2024-09-26T04:12:09.345Z
Learning: In `tests/basic_integration/test.sh`, the user prefers not to refactor the ciphernode addition section to reduce duplication.
Applied to files:
examples/CRISP/scripts/dev_cipher.sh
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: tests/basic_integration/test.sh:81-83
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `tests/basic_integration/test.sh`, it's acceptable to wait indefinitely for the EVM node to start without a timeout, as it's unlikely to fail here.
Applied to files:
examples/CRISP/scripts/dev_cipher.sh
📚 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/test-helpers/src/bin/fake_encrypt.rsexamples/CRISP/crates/zk-inputs/src/lib.rsexamples/CRISP/circuits/src/main.nr
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/server/routes/rounds.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:
examples/CRISP/crates/zk-inputs/src/lib.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/circuits/src/main.nr
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/main.nr
🧬 Code graph analysis (7)
templates/default/program/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_arc(278-300)encode_bfv_params(314-328)
examples/CRISP/server/src/cli/commands.rs (2)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_from_set_arc(206-213)
crates/test-helpers/src/bin/fake_encrypt.rs (1)
crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_from_set_arc(206-213)
examples/CRISP/server/src/server/routes/rounds.rs (1)
crates/bfv-helpers/src/lib.rs (2)
build_bfv_params_from_set_arc(206-213)encode_bfv_params(314-328)
examples/CRISP/crates/zk-inputs/src/lib.rs (3)
examples/CRISP/crates/crisp-constants/src/lib.rs (1)
get_default_paramset(10-13)crates/bfv-helpers/src/lib.rs (1)
build_bfv_params_arc(278-300)examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
new(30-41)with_defaults(45-48)
crates/bfv-helpers/src/lib.rs (3)
crates/wasm/src/lib.rs (1)
from(196-203)examples/CRISP/crates/zk-inputs/src/lib.rs (1)
new(38-46)templates/default/program/src/lib.rs (1)
test(38-78)
crates/wasm/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (2)
get_params_by_str(82-86)get_params_list(89-96)
🔇 Additional comments (22)
examples/CRISP/scripts/dev_cipher.sh (1)
26-27: Verify TRBFV support is functional before enabling the experimental flag.The TODO comment indicates this flag was pending validation. However, based on learnings from prior work (PR #969), TRBFV support in the SDK is intentionally incomplete—the encryption methods throw "Protocol not supported" for TRBFV. Additionally, your PR comments indicate "the circuit is still failing" after this change.
Please verify:
- Whether TRBFV support has been completed since PR #969
- Whether the circuit failures mentioned in your PR comments have been resolved
- Whether other dependent changes (e.g., parameter set configuration, circuit definitions) in this PR address the root cause of the failures
If TRBFV support remains incomplete or the circuit is still failing, this flag should not be enabled in CI/development until the underlying issues are resolved.
examples/CRISP/circuits/src/main.nr (2)
21-41: Parameter migration looks consistent.The polynomial degree reduction (2048 → 512) and moduli count increase (1 → 2) are applied uniformly across all inputs. The derived polynomial sizes for r1is/p1is (1023 = 2×512−1) and r2is/p2is (511 = 512−1) are mathematically correct. Given this significant parameter change, ensure the end-to-end tests validate that encryption, addition, and decryption produce correct results under the new InsecureSet512_10_1 configuration.
Based on past review comments where cedoor confirmed these parameters.
75-92: LGTM! Type instantiations match the confirmed parameter set.The
Greco<512, 2, 36, 36, 2, 6, 6, 4, 10, 36, 10, 36>andCiphertextAddition<512, 2, 36, 36, 36>type parameters align with the InsecureSet512_10_1 configuration and all constructor arguments correctly reference the updated polynomial inputs from the function signature.Based on past review comments where cedoor confirmed these parameters.
.github/workflows/ci.yml (2)
30-52: Job setup is consistent and follows established CI patterns.The installation steps align with existing jobs (
rust_unit,rust_integration), and the toolchain version (1.86.0), Foundry, and solc setup match the repository's standard stack. The working directory and test command are clear and appropriate.
30-52: I need to clarify whether the WASM crate is included in the defaultcargo testcommand or requires special flags/build targets.No issues detected. The CI job configuration is correct and complete.
Based on the verification:
The alloy "node-bindings" feature provides Ethereum execution-layer client bindings, not Node.js binary requirements. These are Rust bindings to interact with Ethereum nodes.
The wasm-bindgen-test crate tests programs compiled to Wasm using the wasm32-unknown-unknown target, which requires an explicit cargo test command with the target flag. The standard
cargo testcommand (as used in the CI job) runs only native target tests and skips WASM crates by default.No build scripts, external tool invocations (Command::new/std::process), or hidden dependencies on Node.js, pnpm, or wasm-pack were found.
The installed dependencies (Rust 1.86.0, Foundry, solc) are sufficient for the unit tests to run successfully.
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (2)
8-9: Circuit size constants verified and mathematically consistent.The constants N=262144 and LOG_N=18 are correct (2^18 = 262144), and verification confirms the insecure parameter set
InsecureSet512_10_1is appropriately documented and scoped to test/example contexts only, with explicit production warnings in place.
14-124: Verification key updated for downsized circuit; verify t1-t4 unchanged values.Circuit size has been reduced from 2^19 (524288) to 2^18 (262144). The circuitSize and logCircuitSize fields correctly reflect this change. Most elliptic curve commitments were regenerated accordingly (ql, qr, qo, q4, qm, qc, qArith, qDeltaRange, qElliptic, qAux, qPoseidon2External, qPoseidon2Internal, s1-s4, id1-id4, lagrangeLast).
However, the table commitments t1, t2, t3, and t4 remain unchanged from the prior state despite the circuit size reduction. While lagrangeFirst and qLookup being unchanged is expected, the t1-t4 values merit investigation:
- VK generation: Confirm this VK was freshly regenerated by the circuit compiler for the new circuit. Any mismatch will cause proof verification to fail or accept invalid proofs.
- Unchanged table commitments: Verify whether t1-t4 being identical across different circuit sizes is expected behavior for your Honk configuration or indicates a regeneration issue.
examples/CRISP/test/crisp.spec.ts (1)
18-19: Formatting-only tweaks; behaviour unchangedAll the marked changes here are trailing commas / minor formatting in valid positions for TypeScript/Node, with no changes to commands, selectors, or control flow. Looks good as-is.
Also applies to: 50-51, 69-70, 97-97, 139-140, 143-144
examples/CRISP/crates/crisp-constants/Cargo.toml (1)
1-10: Newcrisp-constantscrate wiring looks goodWorkspace metadata and the scoped
e3-sdkdependency (default-features = false,features = ["bfv"]) match the intended role of this crate as a BFV-params constants module. No issues from my side.crates/evm/src/helpers.rs (1)
12-15: Simple nonce management integration – please cross-check alias and builder orderThe addition of
SimpleNonceManagerand wiringwith_simple_nonce_management()intocreate_signer_provideris a good improvement to avoid nonce races on write providers.The only things I’d suggest double‑checking:
- That
ConcreteWriteProviderexactly matches the type Alloy’sProviderBuilder::new().with_simple_nonce_management().wallet(wallet)...actually produces (including the newNonceFiller<SimpleNonceManager>layer).- That calling
.with_simple_nonce_management()before.wallet(wallet)is the intended order per Alloy’s builder semantics (and you don’t end up with an unexpected filler composition).If this compiles cleanly and matches Alloy’s examples/tests, the change looks solid.
Also applies to: 99-111, 137-149
crates/bfv-helpers/src/client.rs (1)
128-128: LGTM! Test imports and usage correctly updated.The test code has been properly updated to use
BfvParamSetsinstead of the oldBfvParamsenum, maintaining consistent test behavior.Also applies to: 138-138, 163-163, 195-195, 226-226
crates/test-helpers/src/bin/fake_encrypt.rs (1)
11-11: LGTM! Binary correctly updated to new parameter API.The fake_encrypt binary has been properly updated to use
BfvParamSetsinstead ofBfvParams.Also applies to: 58-58
examples/CRISP/server/src/cli/commands.rs (1)
7-9: Excellent! This addresses the centralization feedback.The CRISP server commands now pull the default parameter set from the centralized
crisp-constantscrate instead of hardcoding the parameter set directly. This makes it easier to iterate on parameter choices and aligns with the PR objective to centralize parameter-set definitions.Based on past review comments suggesting centralization of CRISP parameters.
Also applies to: 331-333
crates/sdk/src/lib.rs (1)
7-14: LGTM! Clean feature-gated module re-exports.The conditional re-exports enable modular usage of SDK components, allowing downstream crates to select only the features they need (bfv, evm, indexer). This is a clean architectural improvement.
crates/test-helpers/src/lib.rs (1)
22-22: LGTM! Test helpers updated to new parameter API.The test helper utilities have been properly updated to use
BfvParamSetsinstead ofBfvParams, maintaining consistency with the broader API changes.Also applies to: 90-90
crates/wasm/src/lib.rs (1)
9-9: LGTM! WASM bindings correctly updated.The WASM bindings have been properly updated to use
BfvParamSetsfor parameter lookups while maintaining the same public API surface. The changes are purely internal and functionally equivalent.Also applies to: 167-167, 184-184
examples/CRISP/server/Cargo.toml (1)
46-48: Verification confirms the dependency configuration is correct.The CRISP server uses all three components provided by the "full" feature:
- evm_helpers: imported in voting.rs, state.rs, rounds.rs, and CLI commands
- bfv_helpers: imported in rounds.rs and CLI commands
- indexer: imported in repo.rs, mod.rs, database.rs, app_data.rs, and indexer.rs
The explicit feature configuration
features=["full"]withdefault-features = falseis the appropriate opt-in pattern for the server's needs, with no unused features.examples/CRISP/crates/crisp-constants/src/lib.rs (1)
10-13: I need to gather more context about the CRISP circuit requirements and any noted failures. Let me search for additional information:Let me search the repository for information about the CRISP circuit implementation and any documented failures:
Let me now search the codebase for CRISP circuit implementation and any related failures or tests:
Based on my verification, I have gathered relevant information about the
InsecureSet512_10_1parameter set and its characteristics. The parameter set is properly defined with:
- Polynomial degree: 512
- Moduli: [0xffffee001, 0xffffc4001] (both 32-bit)
- Plaintext modulus: 10
- Error variance: 3
The noise budget consumption in multiplications is proportional to log(plain_modulus), making the very small plaintext modulus of 10 suitable for CI optimization. However, I cannot definitively verify circuit compatibility or confirm whether the circuit failures mentioned in the PR objectives have been resolved without:
- Access to the specific CRISP circuit requirements and multiplicative depth constraints
- Actual test results or failure logs from the current PR
- Confirmation from the CRISP team on whether the parameter set meets their constraints
Parameter centralization is well-implemented. The code changes successfully consolidate the default parameter configuration as intended. However, verification of actual circuit compatibility and resolution of the noted failures requires manual testing and coordination with the CRISP team, as stated in the original review objectives.
examples/CRISP/crates/zk-inputs/src/lib.rs (2)
11-16: Good centralization of BFV wiring through e3-sdk and crisp-constantsRouting BFV construction via
e3_sdk::bfv_helpers::{build_bfv_params_arc, BfvParamSet, BfvParamSets}andcrisp_constants::get_default_paramsetgives CRISP a single source of truth for parameter sets, and keeps this crate decoupled from the underlying helpers crate layout. This matches the goal of centralizing param configuration and should make switching the default set (or adding secure sets) straightforward.
171-176: DEFAULT_DEGREE now correctly aligned with the 512-degree default param setUpdating
DEFAULT_DEGREEto 512 ensurescreate_vote_vector()matches the degree ofInsecureSet512_10_1, soPlaintext::try_encodeagainstwith_defaults()no longer hits a length mismatch. This resolves the earlier degree/constant divergence for the default CRISP param set.crates/bfv-helpers/src/lib.rs (2)
37-77: Param-set enum and lookup helpers are consistent and strum-friendlyThe new
BfvParamSetsenum captures all the existing sets with clear doc comments and strum attributes, andget_params_by_str/get_params_listprovide a straightforward string-based API for UIs/CLIs. UsingEnumString+IntoStaticStr+EnumIterkeeps parsing and listing in sync with the enum definition, which is a solid approach.Also applies to: 80-96
633-637: Tests cover both BFV and TRBFV mappings via BfvParamSetsThe updated
params_testsnow constructBfvParamSetfromBfvParamSetsand validate:
- Raw field values (
degree,plaintext_modulus,moduli) for the insecure 2048 set.- Round-trips through
build_bfv_params_from_setandbuild_bfv_params_from_set_arc.- ABI encode/decode for both BFV and TRBFV sets, including preservation of
error1_variance.This provides good regression coverage for the new enum + builder API and looks correct as written.
Also applies to: 641-647, 651-657, 661-671, 675-685, 689-702
3 participants
Closes: #990
Goals:
This PR
BfvParamSets(enumeration of different sets)BfvParamSet(single set)BfvParameters(fhe.rs)e3_sdkwith features over usingbfv_helpersdirectlyBfvParamSetsenumInsecureSet512_10_1Summary by CodeRabbit
New Features
Improvements
Chores