refactor: adapt to fhe.rs upstream [skip-line-limit]#1561
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR modernizes RNG usage (unifying on rand::rng()), introduces ToPowerBasisPoly conversions and explicit PowerBasis/NTT-typed Polys across BFV/ZK modules, migrates ZK circuit computation APIs (context/moduli access and CRT indexing), upgrades toolchains/dependencies and CI/Docker, and adds WASM browser smoke tests and benchmark/report refreshes. ChangesDependency and Build Configuration Updates
Polynomial Type System and Helpers
RNG Modernization
BFV Witness and Type System Updates
ZK Circuit Data Structure and Computation Updates
Test Infrastructure and Integration Updates
WASM and Browser Testing
Benchmark and Configuration Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/trbfv/src/calculate_decryption_share.rs (1)
116-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against empty
es_poly_sumbefore modulo indexing.
index % es_poly_sum.len()will panic whenes_poly_sumis empty. Validate once and return an error early.🐛 Proposed fix
let sk_poly_sum = req.sk_poly_sum; let es_poly_sum = req.es_poly_sum; + if es_poly_sum.is_empty() { + bail!("es_poly_sum must contain at least one polynomial"); + } info!("Calculating d_share_poly...");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/trbfv/src/calculate_decryption_share.rs` around lines 116 - 127, Before iterating to build d_share_poly, check that es_poly_sum is non-empty and return an appropriate Error/Result early instead of doing index % es_poly_sum.len(); specifically add a guard above the .into_iter().enumerate() map that validates !es_poly_sum.is_empty() (or returns Err(...) from the surrounding function) so ShareManager::new and the mapping logic never attempt modulo with a zero length; ensure the error type matches the surrounding function's return (e.g., the calculate_decryption_share function's Result) and include a clear message like "empty smudging polynomial list".
🧹 Nitpick comments (3)
crates/cli/Cargo.toml (1)
45-47: 💤 Low valueClarify the need for dual
randversions.Adding
rand08as a separate dependency alongside workspacerandcreates a dual-version situation. This is sometimes necessary for compatibility with dependencies that haven't migrated to newer rand versions. Consider adding a comment explaining which dependency requiresrand 0.8.5to help future maintainers understand this setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/Cargo.toml` around lines 45 - 47, Explain the dual rand dependency by adding an inline comment in Cargo.toml next to the rand08 = { package = "rand", version = "=0.8.5", features = ["std", "std_rng"] } entry that states which crate (name the dependent crate) requires rand 0.8.5 and why we keep the workspace rand alongside it (compatibility/shim for that dependency); ensure the comment mentions the exact dependency name and version constraint so future maintainers understand why rand08 exists and avoid accidental removal or unification with the workspace rand.crates/trbfv/src/calculate_decryption_share.rs (1)
127-131: ⚡ Quick winPrecompute NTT form of
sk_poly_sumonce.The NTT conversion is repeated for each ciphertext. Hoist it out of the loop to reduce per-item overhead.
♻️ Proposed refactor
- let d_share_poly = req + let sk_poly_sum_ntt = sk_poly_sum.into_ntt(); + let d_share_poly = req .ciphertexts .into_iter() .enumerate() .map(|(index, ciphertext)| { @@ share_manager .decryption_share( Arc::new(ciphertext), - sk_poly_sum.clone().into_ntt(), + sk_poly_sum_ntt.clone(), es_poly_sum[es_idx].clone(), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/trbfv/src/calculate_decryption_share.rs` around lines 127 - 131, NTT conversion of sk_poly_sum is being performed inside the loop for every ciphertext; compute sk_poly_sum.into_ntt() once before iterating and reuse it in the call to share_manager.decryption_share to avoid repeated conversions. Specifically, hoist the result of sk_poly_sum.clone().into_ntt() into a local variable (e.g., sk_poly_sum_ntt) outside the loop and pass that variable to share_manager.decryption_share instead of calling into_ntt() repeatedly; keep using es_poly_sum[es_idx] and ciphertext as before.Cargo.toml (1)
155-158: ⚡ Quick winReminder: FHE dependencies must switch to stable references before merge.
Per the PR description's "DO NOT MERGE" notice, these FHE crates are pinned to the
upstream-merge-conflictsbranch. Once fhe.rs PR#61is stabilized and merged, update these to point tomainor a versioned tag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 155 - 158, The Cargo.toml currently pins FHE crates (fhe, fhe-traits, fhe-math, fhe-util) to the temporary branch "upstream-merge-conflicts"; before merging replace these git branch references with stable refs (either branch = "main" or a released semver tag) for each dependency (fhe, fhe-traits, fhe-math, fhe-util) so the project depends on a stable, versioned commit once fhe.rs PR `#61` is merged and published.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/entrypoint/src/start/start.rs`:
- Line 19: Replace the infallible ChaCha20Rng::from_os_rng() call with the
fallible ChaCha20Rng::try_from_os_rng() and propagate/map the Result into anyhow
so startup fails gracefully on OS RNG errors; e.g., change the initialization of
rng (currently Arc::new(Mutex::new(ChaCha20Rng::from_os_rng()))) to use
ChaCha20Rng::try_from_os_rng().map_err/Context and the ? operator so
Arc::new(Mutex::new(...)) receives a seeded RNG on success or returns an anyhow
error on failure.
In `@examples/CRISP/Cargo.toml`:
- Around line 50-53: Replace the git branch pins for the fhe dependencies (fhe,
fhe-traits, fhe-math, fhe-util) with immutable rev pins: replace branch =
"upstream-merge-conflicts" with rev = "<commit-sha>" for each dependency in the
manifest and then run cargo update / cargo generate-lockfile so Cargo.lock
records the resolved ?rev= references rather than branch= entries; ensure the
chosen commit SHAs are the exact commits you want to lock to and update the rev
values accordingly.
In `@templates/default/Cargo.toml`:
- Around line 10-11: The entries for the git dependencies fhe and fhe-traits
currently use branch = "upstream-merge-conflicts" which makes builds
non-reproducible; update both dependency specifications (the fhe and fhe-traits
entries) to include an immutable rev (commit SHA) alongside the git URL (i.e.,
replace or augment branch with rev = "<commit-sha>") so the template pins to a
specific commit and is reproducible.
---
Outside diff comments:
In `@crates/trbfv/src/calculate_decryption_share.rs`:
- Around line 116-127: Before iterating to build d_share_poly, check that
es_poly_sum is non-empty and return an appropriate Error/Result early instead of
doing index % es_poly_sum.len(); specifically add a guard above the
.into_iter().enumerate() map that validates !es_poly_sum.is_empty() (or returns
Err(...) from the surrounding function) so ShareManager::new and the mapping
logic never attempt modulo with a zero length; ensure the error type matches the
surrounding function's return (e.g., the calculate_decryption_share function's
Result) and include a clear message like "empty smudging polynomial list".
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 155-158: The Cargo.toml currently pins FHE crates (fhe,
fhe-traits, fhe-math, fhe-util) to the temporary branch
"upstream-merge-conflicts"; before merging replace these git branch references
with stable refs (either branch = "main" or a released semver tag) for each
dependency (fhe, fhe-traits, fhe-math, fhe-util) so the project depends on a
stable, versioned commit once fhe.rs PR `#61` is merged and published.
In `@crates/cli/Cargo.toml`:
- Around line 45-47: Explain the dual rand dependency by adding an inline
comment in Cargo.toml next to the rand08 = { package = "rand", version =
"=0.8.5", features = ["std", "std_rng"] } entry that states which crate (name
the dependent crate) requires rand 0.8.5 and why we keep the workspace rand
alongside it (compatibility/shim for that dependency); ensure the comment
mentions the exact dependency name and version constraint so future maintainers
understand why rand08 exists and avoid accidental removal or unification with
the workspace rand.
In `@crates/trbfv/src/calculate_decryption_share.rs`:
- Around line 127-131: NTT conversion of sk_poly_sum is being performed inside
the loop for every ciphertext; compute sk_poly_sum.into_ntt() once before
iterating and reuse it in the call to share_manager.decryption_share to avoid
repeated conversions. Specifically, hoist the result of
sk_poly_sum.clone().into_ntt() into a local variable (e.g., sk_poly_sum_ntt)
outside the loop and pass that variable to share_manager.decryption_share
instead of calling into_ntt() repeatedly; keep using es_poly_sum[es_idx] and
ciphertext as before.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f88be63-a1de-45bb-ab71-76ef388668b0
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
Cargo.tomlcircuits/benchmarks/results_insecure_no_agg_micro/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_no_agg_micro/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_no_agg_micro/integration_summary.jsoncircuits/benchmarks/results_insecure_no_agg_micro/report.mdcrates/aggregator/src/publickey_aggregator.rscrates/bfv-client/src/client.rscrates/cli/Cargo.tomlcrates/cli/src/helpers/compile_id.rscrates/crypto/src/cipher.rscrates/entrypoint/src/start/start.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/polynomial/Cargo.tomlcrates/polynomial/src/crt_polynomial.rscrates/polynomial/src/fhe_poly.rscrates/polynomial/src/lib.rscrates/polynomial/src/polynomial.rscrates/test-helpers/src/application.rscrates/test-helpers/src/lib.rscrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/calculate_decryption_key.rscrates/trbfv/src/calculate_decryption_share.rscrates/trbfv/src/calculate_threshold_decryption.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/trbfv/tests/integration.rscrates/wasm/Cargo.tomlcrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/dkg/pk/computation.rscrates/zk-helpers/src/circuits/dkg/pk/sample.rscrates/zk-helpers/src/circuits/dkg/share_computation/sample.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_decryption/sample.rscrates/zk-helpers/src/circuits/dkg/share_encryption/circuit.rscrates/zk-helpers/src/circuits/dkg/share_encryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_encryption/sample.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/circuit.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rscrates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_aggregation/sample.rscrates/zk-helpers/src/circuits/threshold/pk_generation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_generation/sample.rscrates/zk-helpers/src/circuits/threshold/share_decryption/computation.rscrates/zk-helpers/src/circuits/threshold/share_decryption/sample.rscrates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rscrates/zk-helpers/src/circuits/threshold/user_data_encryption/sample.rscrates/zk-helpers/src/circuits/threshold/user_data_encryption/utils.rscrates/zk-helpers/src/math.rscrates/zk-prover/tests/local_e2e_tests.rscrates/zk-prover/tests/slashing_integration_tests.rsexamples/CRISP/Cargo.tomlexamples/CRISP/crates/zk-inputs-wasm/Cargo.tomlexamples/CRISP/crates/zk-inputs/Cargo.tomlexamples/CRISP/crates/zk-inputs/src/ciphertext_addition.rsexamples/CRISP/crates/zk-inputs/src/lib.rsexamples/CRISP/rust-toolchain.tomlpackages/enclave-contracts/deployed_contracts.jsonrust-toolchain.tomltemplates/default/Cargo.tomltemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltests/integration/enclave.config.yaml
💤 Files with no reviewable changes (1)
- crates/polynomial/Cargo.toml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
19-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBump CI
RUST_TOOLCHAINto1.91.1to matchrust-toolchain.toml
.github/workflows/ci.ymlsetsRUST_TOOLCHAIN: 1.86.0, and the workflow’s Rust setup steps pass${{ env.RUST_TOOLCHAIN }}todtolnay/rust-toolchain@stable, while the repo’srust-toolchain.toml(andexamples/CRISP/rust-toolchain.toml) pin1.91.1.- This causes CI to install
1.86.0even though the manifest expects1.91.1, risking mismatches and/or extra rustup downloads.Proposed change
- RUST_TOOLCHAIN: 1.86.0 + RUST_TOOLCHAIN: 1.91.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 19, The CI env var RUST_TOOLCHAIN in the workflow is out of sync with the repo manifest; update the RUST_TOOLCHAIN value in .github/workflows/ci.yml from 1.86.0 to 1.91.1 so the workflow (which passes ${ { env.RUST_TOOLCHAIN } } to the dtolnay/rust-toolchain@stable action) matches rust-toolchain.toml (and examples/CRISP/rust-toolchain.toml); ensure the RUST_TOOLCHAIN entry is the same literal "1.91.1".
🧹 Nitpick comments (1)
crates/Dockerfile (1)
20-20: 💤 Low valueFloating
rust:1.91tag vs pinned1.91.1.
rust:1.91resolves to the newest1.91.xpatch at build time, whilerust-toolchain.tomlandsupport/CRISP Dockerfiles pin1.91.1. For reproducible images, consider pinning the patch here too. Low priority.-FROM rust:1.91 AS ciphernode-builder +FROM rust:1.91.1 AS ciphernode-builder🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/Dockerfile` at line 20, The Dockerfile uses a floating base image tag "rust:1.91" which can resolve to different patch versions; update the FROM line in the Dockerfile (the "FROM rust:1.91 AS ciphernode-builder" instruction) to pin the patch to "rust:1.91.1" so it matches rust-toolchain.toml and other Dockerfiles for reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Line 19: The CI env var RUST_TOOLCHAIN in the workflow is out of sync with the
repo manifest; update the RUST_TOOLCHAIN value in .github/workflows/ci.yml from
1.86.0 to 1.91.1 so the workflow (which passes ${ { env.RUST_TOOLCHAIN } } to
the dtolnay/rust-toolchain@stable action) matches rust-toolchain.toml (and
examples/CRISP/rust-toolchain.toml); ensure the RUST_TOOLCHAIN entry is the same
literal "1.91.1".
---
Nitpick comments:
In `@crates/Dockerfile`:
- Line 20: The Dockerfile uses a floating base image tag "rust:1.91" which can
resolve to different patch versions; update the FROM line in the Dockerfile (the
"FROM rust:1.91 AS ciphernode-builder" instruction) to pin the patch to
"rust:1.91.1" so it matches rust-toolchain.toml and other Dockerfiles for
reproducible builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6223f317-5df0-4033-9bd7-388f00abc6b3
📒 Files selected for processing (7)
.github/workflows/ci.ymlcrates/Dockerfilecrates/entrypoint/src/start/start.rscrates/net/tests/Dockerfilecrates/support/Dockerfileexamples/CRISP/server/Dockerfilepackages/enclave-contracts/deployed_contracts.json
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/entrypoint/src/start/start.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1033-1034: The workflow is using an unpinned action reference
browser-actions/setup-chrome@v1; replace both occurrences of
browser-actions/setup-chrome@v1 with the same full commit SHA pin
(browser-actions/setup-chrome@<commit-sha>) to avoid mutable tags. Locate the
two uses of the setup-chrome action and update them to the exact commit SHA
corresponding to v1 (use the current v1 commit SHA from the action repo) so both
places use browser-actions/setup-chrome@<sha> instead of `@v1`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daf83c47-418c-42b3-b11c-65b7743ff467
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.github/workflows/ci.ymlCargo.tomlREADME.mdcrates/aggregator/src/threshold_plaintext_aggregator.rscrates/keyshare/Cargo.tomlcrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/support/Cargo.tomlcrates/trbfv/src/calculate_threshold_decryption.rscrates/trbfv/src/gen_esi_sss.rscrates/trbfv/src/helpers.rscrates/wasm/.cargo/config.tomlcrates/wasm/package.jsoncrates/wasm/scripts/browser-smoke.mjscrates/wasm/smoke.htmlcrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rsdocs/pages/quick-start.mdxexamples/CRISP/Dockerfileexamples/CRISP/packages/crisp-zk-inputs/package.jsonexamples/CRISP/packages/crisp-zk-inputs/scripts/browser-smoke.mjsexamples/CRISP/packages/crisp-zk-inputs/smoke.htmlexamples/CRISP/server/Cargo.tomltemplates/default/flake.nix
✅ Files skipped from review due to trivial changes (6)
- templates/default/flake.nix
- examples/CRISP/server/Cargo.toml
- docs/pages/quick-start.mdx
- crates/wasm/.cargo/config.toml
- crates/wasm/smoke.html
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/trbfv/src/calculate_threshold_decryption.rs
- Cargo.toml
- crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs
ctrlc03
added a commit
that referenced
this pull request
Jun 1, 2026
The fhe.rs bump (#1561) moved fhe/fhe-math to 0.2.0 and fhe-traits/fhe-util to 0.1.1 (git rev a92478b3) but only touched templates/default/flake.nix, leaving the root flake's cargoLock.outputHashes keyed by the old -0.1.0-beta.7 versions. nix build failed with "A hash was specified for fhe-0.1.0-beta.7, but there is no corresponding git dependency". Rename the four keys to match Cargo.lock. NOTE: fheHash (the FOD hash value) still needs updating for the new rev — the next `nix build .#default` will report the correct `got: sha256-...` to paste into flake.nix line 18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
WE NEED TO STABILIZE AND MERGE gnosisguild/fhe.rs#61. THEN, SWITCH TO MAIN FHE.RS AND MOVE ON.
NB. THERE ARE BREAKING CHANGES SINCE WE MOVE TO RUST 1.91.1, RAND 0.9 AND NEW FHE.RS APIS!
Summary by CodeRabbit
New Features
Chores