feat: optimize pipeline for nodes_fold and reuse of vk [skip-line-limit]#1581
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (2)
crates/aggregator/src/actors/publickey_aggregator.rs (1)
586-697: 💤 Low value
BTreeSet::iter().nth()has O(n) complexity per call.Line 619 uses
honest_party_ids.iter().nth(next_slot as usize)which is O(H) for each fold step. Across H steps, this results in O(H²) total iteration overhead. For typical committee sizes this is likely acceptable, but if H grows large, consider caching the sorted party IDs as aVec<u64>once when enteringGeneratingC5Proofto enable O(1) indexed access.🤖 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/aggregator/src/actors/publickey_aggregator.rs` around lines 586 - 697, The code uses BTreeSet::iter().nth(next_slot) in try_dispatch_nodes_fold_step (within PublicKeyAggregatorState::GeneratingC5Proof) which yields O(H) per lookup causing O(H²) behavior; change the state to cache a Vec of sorted honest IDs (e.g., add honest_party_ids_vec: Vec<u64> to PublicKeyAggregatorState::GeneratingC5Proof or populate an existing vector when entering that state) and replace honest_party_ids.iter().nth(next_slot as usize) with direct indexed access honest_party_ids_vec[next_slot as usize]; ensure the Vec is created once (when transitioning into GeneratingC5Proof) and used everywhere that currently reads honest_party_ids by index.crates/events/src/enclave_event/compute_request/zk.rs (1)
76-91: 💤 Low valueType consistency:
slot_index: u32vstotal_slots: usize.The mixed integer types (
slot_index: u32andtotal_slots: usize) could cause subtle issues on platforms whereusizediffers from 32 bits, and requires explicit casts at comparison sites. Consider aligning both to the same type (eitheru32orusize) for consistency.🤖 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/events/src/enclave_event/compute_request/zk.rs` around lines 76 - 91, The struct NodesFoldStepRequest mixes slot_index: u32 and total_slots: usize which can cause casts and platform differences; pick a single integer type (recommend usize for counts/indexing or u32 if protocol guarantees 32-bit) and make both fields the same type (change slot_index to usize or total_slots to u32), then update all constructors, builders, serde/Serialize/Deserialize usage, and any comparisons or arithmetic in functions that reference NodesFoldStepRequest::{slot_index,total_slots} to use the unified type to avoid repeated casts and platform-dependent bugs.
🤖 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/aggregator/src/domain/publickey_aggregation.rs`:
- Around line 209-214: The new required field nodes_fold_completed_slots (u32)
breaks deserialization of old snapshots; fix by making deserialization
backward-compatible: either add #[serde(default)] to nodes_fold_completed_slots
so missing values default to 0, or change its type to Option<u32> and treat None
as 0 at runtime. Update the GeneratingC5Proof variant (and any constructors/uses
of nodes_fold_completed_slots) to handle the default/Option (e.g., unwrap_or(0))
so existing logic continues to work.
---
Nitpick comments:
In `@crates/aggregator/src/actors/publickey_aggregator.rs`:
- Around line 586-697: The code uses BTreeSet::iter().nth(next_slot) in
try_dispatch_nodes_fold_step (within
PublicKeyAggregatorState::GeneratingC5Proof) which yields O(H) per lookup
causing O(H²) behavior; change the state to cache a Vec of sorted honest IDs
(e.g., add honest_party_ids_vec: Vec<u64> to
PublicKeyAggregatorState::GeneratingC5Proof or populate an existing vector when
entering that state) and replace honest_party_ids.iter().nth(next_slot as usize)
with direct indexed access honest_party_ids_vec[next_slot as usize]; ensure the
Vec is created once (when transitioning into GeneratingC5Proof) and used
everywhere that currently reads honest_party_ids by index.
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 76-91: The struct NodesFoldStepRequest mixes slot_index: u32 and
total_slots: usize which can cause casts and platform differences; pick a single
integer type (recommend usize for counts/indexing or u32 if protocol guarantees
32-bit) and make both fields the same type (change slot_index to usize or
total_slots to u32), then update all constructors, builders,
serde/Serialize/Deserialize usage, and any comparisons or arithmetic in
functions that reference NodesFoldStepRequest::{slot_index,total_slots} to use
the unified type to avoid repeated casts and platform-dependent bugs.
🪄 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: c63a049b-3064-4935-8d7a-5120ecc3cb91
📒 Files selected for processing (11)
crates/aggregator/src/actors/publickey_aggregator.rscrates/aggregator/src/domain/publickey_aggregation.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/multithread/src/multithread.rscrates/zk-prover/src/circuits/aggregation/c3_accumulator.rscrates/zk-prover/src/circuits/aggregation/c6_accumulator.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rscrates/zk-prover/src/lib.rscrates/zk-prover/tests/fold_accumulators_e2e_tests.rs
2 participants
Summary by CodeRabbit
New Features
Refactor