feat: add proof of correct aggregration#1374
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a multi-stage public-key aggregation flow: collects optional C1 proofs, verifies shares (C1), performs threshold aggregation, generates a C5 PK-aggregation ZK proof, and threads a BFV preset and ZK backend through the aggregator, multithread prover, events, builder, tests, and configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Pka as PublicKeyAggregator
participant Bus as Extension/Bus
participant Zk as Multithread ZK Prover
participant State as Aggregator State
rect rgba(100,150,255,0.5)
Note over Pka,State: Collecting phase
Pka->>Pka: add_keyshare(keyshare, c1_proof?)
Pka->>State: accumulate keyshares + c1_proofs
Note over Pka: on threshold_n reached -> trigger C1 verification
end
rect rgba(150,100,255,0.5)
Note over Pka,Zk: C1 verification
Pka->>Bus: dispatch_c1_verification(request with proofs)
Bus->>Zk: VerifyShareProofs request
Zk-->>Bus: VerifyShareProofs response (honest/dishonest)
Bus->>Pka: deliver verification response
Pka->>Pka: handle_c1_verification_response()
Pka->>State: require >= threshold_m honest -> aggregate keyshares
end
rect rgba(255,150,100,0.5)
Note over Pka,Zk: C5 PK-aggregation proof
Pka->>Bus: dispatch_c5_proof(pk, keyshare_bytes, params_preset)
Bus->>Zk: PkAggregation request
Zk->>Zk: Generate C5 proof (circuit)
Zk-->>Bus: PkAggregation response (proof)
Bus->>Pka: deliver proof
Pka->>Pka: handle_c5_proof_response() -> Publish PublicKeyAggregated
Pka->>State: transition to Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
templates/default/enclave.config.yaml (1)
7-19: Consider generatingdeploy_blockvalues to avoid config drift.Lines 7, 10, 16, and 19 are hand-maintained and can silently drift when deployment order changes. A generated template (from deployment artifacts) would be safer long-term.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/enclave.config.yaml` around lines 7 - 19, The static deploy_block values (deploy_block keys under ciphernode_registry, bonding_registry, fee_token, e3_program) can drift; change the template generation so deploy_block is populated from the deployment artifacts instead of being hand-edited: update the template generator (or add a small script run during build) to read the deployment manifest/ABI output (the artifacts that record addresses and block numbers) and inject the correct deploy_block for each registry/key into the enclave.config.yaml template (ensuring it fills deploy_block for ciphernode_registry, bonding_registry, fee_token, e3_program), and remove/replace hard-coded numbers in the template so the file is produced from the artifact data automatically.crates/multithread/src/multithread.rs (1)
262-325: Add upfront structural validation for PK aggregation inputs.Before deserialization/proving, validate
n/h/thresholdrelationships and keyshare count consistency to fail fast with clear errors.Proposed hardening
fn handle_pk_aggregation_proof( prover: &ZkProver, req: PkAggregationProofRequest, request: ComputeRequest, ) -> Result<ComputeResponse, ComputeRequestError> { + if req.committee_n == 0 + || req.committee_h == 0 + || req.committee_h > req.committee_n + || req.committee_threshold == 0 + || req.committee_threshold > req.committee_h + { + return Err(make_zk_error( + &request, + format!( + "Invalid committee values: n={}, h={}, threshold={}", + req.committee_n, req.committee_h, req.committee_threshold + ), + )); + } + if req.keyshare_bytes.len() != req.committee_h { + return Err(make_zk_error( + &request, + format!( + "Expected {} keyshares, got {}", + req.committee_h, + req.keyshare_bytes.len() + ), + )); + } + // 1. Build threshold BFV parameters from preset let (threshold_params, _dkg_params) = build_pair_for_preset(req.params_preset.clone()) .map_err(|e| make_zk_error(&request, format!("build_pair_for_preset: {}", e)))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 262 - 325, Add upfront validation in handle_pk_aggregation_proof before deserializing keyshares: verify req.committee_threshold > 0 and that req.committee_threshold <= req.committee_h && req.committee_h <= req.committee_n, and that req.keyshare_bytes.len() == req.committee_n (or the expected count per your protocol) — if any check fails return an early error (use make_zk_error(&request, format!(...)) or ComputeRequestError consistent with existing error handling) so the function fails fast with a clear message; place this block just before the keyshare deserialization loop that references pk0_shares and use the same request/context (request, req) for error construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 151-154: The code currently derives sender_party_id from the
iterator index (idx as u64) which is wrong; instead use the actual party
identifier embedded in the proof or upstream tuple so proofs remain bound to
their true sender. Locate the closure that builds PartyProofsToVerify (the
filter_map producing PartyProofsToVerify { sender_party_id: idx as u64,
signed_proofs: vec![proof.clone()] }) and replace the idx-based assignment with
the real party id from the proof (e.g., proof.sender_party_id or proof.party_id)
or from the surrounding upstream data structure that carries the party id;
ensure PartyProofsToVerify.sender_party_id is populated from that authoritative
field and update any upstream mapping to pass the party id through if necessary.
- Around line 148-163: The current construction of party_proofs using
c1_proofs.iter().enumerate().filter_map(...) silently drops None entries so
keyshares with missing C1 proofs bypass verification; update the logic in the
c1_proofs -> party_proofs conversion to explicitly fail when any proof_opt is
None (for example, iterate with enumerate and return an Err if
proof_opt.is_none()), ensuring missing proofs cause an early Err instead of
being omitted, and keep references to PartyProofsToVerify and honest_keyshares
intact so only fully-proven parties proceed.
- Around line 278-281: The code currently hardcodes committee_threshold: 0 when
constructing the statement (the fields committee_n, committee_h,
committee_threshold), which produces an invalid C5 input; instead populate
committee_threshold with the actual threshold value resolved from your
preset/handler (e.g., use preset.committee_threshold or the resolved variable
from the handler, or compute it via resolve_threshold(committee_h.len()) and
pass that variable into the struct). Replace committee_threshold: 0 with the
resolved threshold variable so the downstream proof handler receives the correct
value.
- Around line 339-347: Ensure this actor only processes ComputeResponse events
for its own e3_id by adding an e3_id guard before matching on response.response:
compare response.e3_id (or the event’s e3_id field) to this aggregator’s
self.e3_id (or the ec/econtext e3 id if that's the authoritative source) and
return Ok(()) early if they differ; then proceed to match ComputeResponseKind
and call handle_c1_verification_response(...) and handle_c5_proof_response(...)
as before.
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 466-475: The code currently unconditionally enables
PublicKeyAggregatorExtension with the insecure BfvPreset::InsecureThreshold512
and does not check for a zk backend; update the builder to fail fast or require
configuration: ensure that before calling PublicKeyAggregatorExtension::create
(and e3_builder.with) you (1) check that a zk_backend is present/configured and
return an error if missing, and (2) disallow using
BfvPreset::InsecureThreshold512 in non-test contexts by making the preset
configurable via the builder API (or validate a provided preset) and returning
an explicit error if the insecure preset is selected; keep references to
ensure_multithread(&bus), PublicKeyAggregatorExtension::create,
BfvPreset::InsecureThreshold512, and e3_builder.with to locate and change the
logic.
In `@crates/events/src/enclave_event/publickey_aggregated.rs`:
- Around line 22-23: The Display implementation currently formats the
pk_aggregation_proof field with a debug formatter and will emit the entire proof
payload; update the Display (impl Display for the enclosing type) to avoid
printing the full Proof by replacing the {:?} usage for pk_aggregation_proof
with a concise/redacted representation (e.g., show "present"/"none", a short
hex/hash or length, or call a summary method on Proof) so logs no longer contain
the full proof data while keeping clear indication that the proof exists.
---
Nitpick comments:
In `@crates/multithread/src/multithread.rs`:
- Around line 262-325: Add upfront validation in handle_pk_aggregation_proof
before deserializing keyshares: verify req.committee_threshold > 0 and that
req.committee_threshold <= req.committee_h && req.committee_h <=
req.committee_n, and that req.keyshare_bytes.len() == req.committee_n (or the
expected count per your protocol) — if any check fails return an early error
(use make_zk_error(&request, format!(...)) or ComputeRequestError consistent
with existing error handling) so the function fails fast with a clear message;
place this block just before the keyshare deserialization loop that references
pk0_shares and use the same request/context (request, req) for error
construction.
In `@templates/default/enclave.config.yaml`:
- Around line 7-19: The static deploy_block values (deploy_block keys under
ciphernode_registry, bonding_registry, fee_token, e3_program) can drift; change
the template generation so deploy_block is populated from the deployment
artifacts instead of being hand-edited: update the template generator (or add a
small script run during build) to read the deployment manifest/ABI output (the
artifacts that record addresses and block numbers) and inject the correct
deploy_block for each registry/key into the enclave.config.yaml template
(ensuring it fills deploy_block for ciphernode_registry, bonding_registry,
fee_token, e3_program), and remove/replace hard-coded numbers in the template so
the file is produced from the artifact data automatically.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/aggregator/Cargo.tomlcrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/entrypoint/src/start/aggregator_start.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rsexamples/CRISP/test/crisp.spec.tstemplates/default/enclave.config.yamltemplates/default/server/index.ts
💤 Files with no reviewable changes (1)
- templates/default/server/index.ts
ctrlc03
force-pushed
the
feat/t4-integration
branch
from
0e1f125 to
2837456
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aggregator/src/publickey_aggregator.rs (1)
427-447: Snapshot-then-mutate pattern for c1_proofs.The code snapshots
c1_proofsbefore mutation (lines 428-435) to have a complete list when dispatching verification (line 445). This works but is slightly fragile since it relies on the snapshot including the newly added proof.Consider: The snapshot adds the new proof manually (line 431), which mirrors what
add_keysharedoes. This duplication is functional but could diverge if logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 427 - 447, The current code creates c1_proofs_snapshot by cloning and manually appending c1_proof before calling add_keyshare, which duplicates logic and may diverge; instead, call add_keyshare(pubkey, node, c1_proof, &ec) first and then read the updated proofs from self.state (match PublicKeyAggregatorState::Collecting { c1_proofs, .. } or VerifyingC1 { .. } as appropriate) to obtain the authoritative list to pass into dispatch_c1_verification(&c1_proofs, ec), or modify add_keyshare to return the updated c1_proofs (or a flag/Option indicating transition) and use that return value to drive dispatch_c1_verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 427-447: The current code creates c1_proofs_snapshot by cloning
and manually appending c1_proof before calling add_keyshare, which duplicates
logic and may diverge; instead, call add_keyshare(pubkey, node, c1_proof, &ec)
first and then read the updated proofs from self.state (match
PublicKeyAggregatorState::Collecting { c1_proofs, .. } or VerifyingC1 { .. } as
appropriate) to obtain the authoritative list to pass into
dispatch_c1_verification(&c1_proofs, ec), or modify add_keyshare to return the
updated c1_proofs (or a flag/Option indicating transition) and use that return
value to drive dispatch_c1_verification.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/aggregator/Cargo.tomlcrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/entrypoint/src/start/aggregator_start.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rsexamples/CRISP/test/crisp.spec.tstemplates/default/enclave.config.yamltemplates/default/server/index.ts
💤 Files with no reviewable changes (1)
- templates/default/server/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/events/src/enclave_event/compute_request/mod.rs
- examples/CRISP/test/crisp.spec.ts
- crates/aggregator/Cargo.toml
- crates/entrypoint/src/start/aggregator_start.rs
ctrlc03
force-pushed
the
feat/t4-integration
branch
from
2837456 to
e126556
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tests/tests/integration.rs (1)
1270-1276:⚠️ Potential issue | 🟡 MinorThese assertions hardcode
pk_aggregation_proof: None, which is likely stale for C5-enabled flow.At Line 1275 and Line 1318, prefer asserting field-wise with
is_some()(or pattern matching) instead of full-struct equality withNone.Also applies to: 1313-1319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 1270 - 1276, The test currently constructs/compares a PublicKeyAggregated instance with pk_aggregation_proof: None which is stale for C5-enabled flows; update the assertions that reference PublicKeyAggregated and pk_aggregation_proof (e.g., the instances built around test_pubkey/public_key_hash/E3id and nodes: OrderedSet::from(eth_addrs.clone())) to check the pk_aggregation_proof field presence instead of exact equality—use field-wise assertions (e.g., public.pk_aggregation_proof.is_some() or pattern-match PublicKeyAggregated { pk_aggregation_proof: Some(_), .. } ) and keep other fields asserted as before so tests no longer fail when a proof is present.
♻️ Duplicate comments (5)
crates/events/src/enclave_event/publickey_aggregated.rs (1)
26-29:⚠️ Potential issue | 🟠 MajorAvoid dumping full proof data in
Display.At Line 28,
{:?}will includepk_aggregation_proofcontent, which can heavily bloat logs.Proposed fix
impl Display for PublicKeyAggregated { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self) + write!( + f, + "PublicKeyAggregated {{ e3_id: {:?}, nodes: {}, pk_aggregation_proof: {} }}", + self.e3_id, + self.nodes.len(), + self.pk_aggregation_proof.is_some() + ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/publickey_aggregated.rs` around lines 26 - 29, The Display impl for PublicKeyAggregated currently uses write!(f, "{:?}", self) which dumps the full struct (including pk_aggregation_proof) into logs; change the fmt method on the Display impl for PublicKeyAggregated to avoid printing pk_aggregation_proof directly — instead format only concise fields (e.g., epoch_index, public_key_aggregated identifier or short hex/prefix, and/or the proof length) and redact or shorten pk_aggregation_proof to a fixed-size summary so logs are not bloated.crates/aggregator/src/publickey_aggregator.rs (3)
303-310:⚠️ Potential issue | 🔴 CriticalDo not send
committee_threshold: 0into C5 proof generation.At Line 309, this produces an invalid/weak committee statement for
PkAggregationProofRequest.Proposed fix
- committee_threshold: 0, // Will be resolved from preset in handler + committee_threshold: threshold_m,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 303 - 310, The PkAggregationProofRequest is being constructed with committee_threshold: 0 which yields an invalid/weak committee statement; instead compute the real threshold from the params preset before building the request and pass that value. Concretely, read the committee threshold from self.params_preset (e.g., call the preset method that returns the committee threshold for the given committee size or preset) into a local variable (e.g., committee_threshold) and replace committee_threshold: 0 in the ZkRequest::PkAggregation(PkAggregationProofRequest { ... }) construction with that computed value so the request carries the correct threshold.
363-376:⚠️ Potential issue | 🔴 CriticalScope
ComputeResponsehandling to this actor’se3_id.Without an
e3_idguard before Line 368 matching, this actor can consume responses from other flows and transition incorrectly.Proposed fix
fn handle_compute_response( &mut self, response: ComputeResponse, ec: EventContext<Sequenced>, ) -> Result<()> { + if response.e3_id != self.e3_id { + return Ok(()); + } + match response.response { ComputeResponseKind::Zk(ZkResponse::VerifyShareProofs(data)) => { self.handle_c1_verification_response(data, ec)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 363 - 376, In handle_compute_response, scope incoming ComputeResponse to this actor's e3_id by returning early when the response's e3_id (or flow identifier field on ComputeResponse) does not equal self.e3_id; add an explicit guard at the start of handle_compute_response (before matching response.response) that compares response.e3_id (or the appropriate flow id field on ComputeResponse) to self.e3_id and returns Ok(()) on mismatch so only responses for this actor proceed to handle_c1_verification_response and handle_c5_proof_response.
154-160:⚠️ Potential issue | 🔴 CriticalBind C1 verification to authoritative party IDs, not collection index.
At Line 158,
sender_party_id: idx as u64ties proof identity to arrival order, which can mis-associate proofs and parties when ordering changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 154 - 160, The loop over c1_proofs currently uses the enumeration index for sender_party_id (sender_party_id: idx as u64), which binds identity to arrival order; instead extract and use the authoritative party ID from the proof itself (e.g., proof.sender_party_id or the proof's canonical identifier) when constructing PartyProofsToVerify so proofs remain associated with the correct party regardless of ordering; update the loop that builds party_proofs (the c1_proofs.iter().enumerate() block and PartyProofsToVerify construction) to read the ID from the proof object rather than using idx.crates/ciphernode-builder/src/ciphernode_builder.rs (1)
464-475:⚠️ Potential issue | 🟠 MajorFail fast if
pubkey_aggis enabled without a ZK backend.This branch enables
PublicKeyAggregatorExtension, but withoutzk_backendthe C1/C5 proof flow cannot complete reliably.Proposed fix
if self.pubkey_agg { info!("Setting up PublicKeyAggregationExtension"); // Ensure multithread worker is available for C1 verification and C5 proof generation let _ = self.ensure_multithread(&bus); + if self.zk_backend.is_none() { + return Err(anyhow::anyhow!( + "ZK backend is required when public key aggregation is enabled" + )); + } // TODO: Make BfvPreset configurable via builder method. // Currently hardcoded to InsecureThreshold512 for C5 proof generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 464 - 475, If self.pubkey_agg is true, fail fast when there is no ZK backend configured: before calling PublicKeyAggregatorExtension::create (and before ensure_multithread / e3_builder.with), check the builder's zk_backend (self.zk_backend) and return an Err (or propagate an appropriate construction error) with a clear message that pubkey_agg requires a ZK backend; do this in the same branch where pubkey_agg is handled so the builder doesn't proceed to create the extension without zk_backend configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/multithread/src/multithread.rs`:
- Around line 264-306: In handle_pk_aggregation_proof validate committee
invariants and keyshare count before any deserialization or circuit
construction: check req.committee_n, req.committee_h, req.committee_threshold
satisfy expected relationships (e.g., 0 < h <= n, threshold <= n and > 0,
threshold <= h as applicable) and ensure req.keyshare_bytes.len() ==
req.committee_n (or the required number of shares) and return an early Err via
make_zk_error(&request, "...") on violation; perform these checks at the top of
handle_pk_aggregation_proof before build_pair_for_preset/deserialize loops so
PkAggregationCircuitData, pk0_shares population, and PublicKey::from_bytes are
only executed for well-formed inputs.
In `@examples/CRISP/test/crisp.spec.ts`:
- Line 145: The WAIT constant is computed from process.env.E3_DURATION without
validation, which yields NaN if the env var is absent; update the initialization
of WAIT (the const named WAIT) to first guard E3_DURATION: check that
process.env.E3_DURATION is defined and a valid integer (e.g., use
Number.isFinite(Number.parseInt(...)) or a regex) and either provide a safe
default (like 0) or throw a clear error, then compute WAIT using the
validated/parsed value so parseInt never receives undefined and WAIT cannot
become NaN.
---
Outside diff comments:
In `@crates/tests/tests/integration.rs`:
- Around line 1270-1276: The test currently constructs/compares a
PublicKeyAggregated instance with pk_aggregation_proof: None which is stale for
C5-enabled flows; update the assertions that reference PublicKeyAggregated and
pk_aggregation_proof (e.g., the instances built around
test_pubkey/public_key_hash/E3id and nodes: OrderedSet::from(eth_addrs.clone()))
to check the pk_aggregation_proof field presence instead of exact equality—use
field-wise assertions (e.g., public.pk_aggregation_proof.is_some() or
pattern-match PublicKeyAggregated { pk_aggregation_proof: Some(_), .. } ) and
keep other fields asserted as before so tests no longer fail when a proof is
present.
---
Duplicate comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 303-310: The PkAggregationProofRequest is being constructed with
committee_threshold: 0 which yields an invalid/weak committee statement; instead
compute the real threshold from the params preset before building the request
and pass that value. Concretely, read the committee threshold from
self.params_preset (e.g., call the preset method that returns the committee
threshold for the given committee size or preset) into a local variable (e.g.,
committee_threshold) and replace committee_threshold: 0 in the
ZkRequest::PkAggregation(PkAggregationProofRequest { ... }) construction with
that computed value so the request carries the correct threshold.
- Around line 363-376: In handle_compute_response, scope incoming
ComputeResponse to this actor's e3_id by returning early when the response's
e3_id (or flow identifier field on ComputeResponse) does not equal self.e3_id;
add an explicit guard at the start of handle_compute_response (before matching
response.response) that compares response.e3_id (or the appropriate flow id
field on ComputeResponse) to self.e3_id and returns Ok(()) on mismatch so only
responses for this actor proceed to handle_c1_verification_response and
handle_c5_proof_response.
- Around line 154-160: The loop over c1_proofs currently uses the enumeration
index for sender_party_id (sender_party_id: idx as u64), which binds identity to
arrival order; instead extract and use the authoritative party ID from the proof
itself (e.g., proof.sender_party_id or the proof's canonical identifier) when
constructing PartyProofsToVerify so proofs remain associated with the correct
party regardless of ordering; update the loop that builds party_proofs (the
c1_proofs.iter().enumerate() block and PartyProofsToVerify construction) to read
the ID from the proof object rather than using idx.
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 464-475: If self.pubkey_agg is true, fail fast when there is no ZK
backend configured: before calling PublicKeyAggregatorExtension::create (and
before ensure_multithread / e3_builder.with), check the builder's zk_backend
(self.zk_backend) and return an Err (or propagate an appropriate construction
error) with a clear message that pubkey_agg requires a ZK backend; do this in
the same branch where pubkey_agg is handled so the builder doesn't proceed to
create the extension without zk_backend configured.
In `@crates/events/src/enclave_event/publickey_aggregated.rs`:
- Around line 26-29: The Display impl for PublicKeyAggregated currently uses
write!(f, "{:?}", self) which dumps the full struct (including
pk_aggregation_proof) into logs; change the fmt method on the Display impl for
PublicKeyAggregated to avoid printing pk_aggregation_proof directly — instead
format only concise fields (e.g., epoch_index, public_key_aggregated identifier
or short hex/prefix, and/or the proof length) and redact or shorten
pk_aggregation_proof to a fixed-size summary so logs are not bloated.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/aggregator/Cargo.tomlcrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/entrypoint/src/start/aggregator_start.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rsexamples/CRISP/test/crisp.spec.tstemplates/default/enclave.config.yamltemplates/default/server/index.ts
💤 Files with no reviewable changes (1)
- templates/default/server/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/aggregator/Cargo.toml
- crates/aggregator/src/ext.rs
- crates/events/src/enclave_event/compute_request/mod.rs
- crates/entrypoint/src/start/aggregator_start.rs
ctrlc03
force-pushed
the
feat/t4-integration
branch
from
e126556 to
4696d0e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
crates/aggregator/src/publickey_aggregator.rs (3)
154-160:⚠️ Potential issue | 🔴 CriticalDo not derive
sender_party_idfrom insertion index.Binding
sender_party_idtoidxcan mis-associate proofs with the wrong party identity. Use the authoritative party ID carried by the proof/event metadata and persist that mapping through collection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 154 - 160, The loop currently sets PartyProofsToVerify.sender_party_id using the enumeration index (idx) which can mis-associate proofs; instead extract the authoritative party ID from the proof/event metadata (e.g., a field like proof.sender_party_id, proof.party_id, or proof.metadata.party_id) and use that when constructing PartyProofsToVerify in the c1_proofs iteration that fills party_proofs; ensure you handle the Option case (Some(proof)) by reading the party id from the proof before cloning/consuming it and propagate that mapping so sender_party_id is sourced from the proof metadata rather than idx.
363-376:⚠️ Potential issue | 🔴 CriticalScope compute responses to this aggregator’s
e3_id.Without an
e3_idguard, this actor can consumeComputeResponseevents from other E3 flows.Proposed fix
fn handle_compute_response( &mut self, response: ComputeResponse, ec: EventContext<Sequenced>, ) -> Result<()> { + if response.e3_id != self.e3_id { + return Ok(()); + } + match response.response { ComputeResponseKind::Zk(ZkResponse::VerifyShareProofs(data)) => { self.handle_c1_verification_response(data, ec) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 363 - 376, The handler handle_compute_response currently processes any ComputeResponse; add a guard that only proceeds if the response's e3_id matches this aggregator's current e3 id (e.g., compare response.e3_id to self.state.e3_id or the appropriate stored field); if they differ, return Ok(()) to ignore the event. Apply this check before matching on response.response so c1/c5 handling (handle_c1_verification_response and handle_c5_proof_response) only runs for responses belonging to this aggregator's e3 flow.
303-310:⚠️ Potential issue | 🔴 Critical
committee_thresholdmust use the actual threshold value.Sending
committee_threshold: 0builds an invalid C5 statement input.Proposed fix
let request = ComputeRequest::zk( ZkRequest::PkAggregation(PkAggregationProofRequest { keyshare_bytes: honest_keyshares, aggregated_pk_bytes: ArcBytes::from_bytes(&pubkey), params_preset: self.params_preset.clone(), committee_n: committee_h, // N = all parties in this aggregation committee_h, - committee_threshold: 0, // Will be resolved from preset in handler + committee_threshold: threshold_m, }), CorrelationId::new(), self.e3_id.clone(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 303 - 310, The PkAggregation request is setting committee_threshold: 0 which produces an invalid C5 statement; instead compute and pass the real threshold before constructing ZkRequest::PkAggregation — locate the PkAggregationProofRequest construction and replace the hardcoded 0 with the actual threshold derived from the preset/committee (e.g. resolve from self.params_preset or from the committee_h/Committee type via its threshold getter) so committee_threshold contains the real threshold value used for this committee.crates/ciphernode-builder/src/ciphernode_builder.rs (1)
466-475:⚠️ Potential issue | 🟠 MajorRequire ZK backend (and avoid fixed insecure preset) when enabling public-key aggregation.
This branch can enable
PublicKeyAggregatorExtensionwithout a configured prover, which causes runtime C1/C5 failures. Also, hardcodingInsecureThreshold512here should be replaced by builder-provided configuration.Proposed hardening
if self.pubkey_agg { info!("Setting up PublicKeyAggregationExtension"); // Ensure multithread worker is available for C1 verification and C5 proof generation let _ = self.ensure_multithread(&bus); + if self.zk_backend.is_none() { + return Err(anyhow::anyhow!( + "ZK backend is required when public key aggregation is enabled" + )); + } // TODO: Make BfvPreset configurable via builder method. // Currently hardcoded to InsecureThreshold512 for C5 proof generation. // Production deployments should use the appropriate threshold preset. let aggregator_preset = BfvPreset::InsecureThreshold512; e3_builder = e3_builder.with(PublicKeyAggregatorExtension::create( &bus, aggregator_preset, )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 466 - 475, The current code unconditionally enables PublicKeyAggregatorExtension via PublicKeyAggregatorExtension::create with a hardcoded BfvPreset::InsecureThreshold512 and may run without a configured prover, causing runtime C1/C5 failures; update the builder (where e3_builder is composed) to (1) require/verify that a ZK prover/backend is configured before calling PublicKeyAggregatorExtension::create (use the same config/field used to register the prover), and (2) replace the fixed BfvPreset::InsecureThreshold512 with a preset passed through the builder API (add a builder field like aggregator_preset and a setter method) so PublicKeyAggregatorExtension::create uses the configured preset; also ensure ensure_multithread(&bus) still runs but do not enable aggregation unless the prover and preset are valid.crates/events/src/enclave_event/publickey_aggregated.rs (1)
22-29:⚠️ Potential issue | 🟠 MajorRedact proof content from
Displayoutput.With
pk_aggregation_proofnow included,write!(f, "{:?}", self)can emit full proof payloads and bloat logs.Proposed fix
impl Display for PublicKeyAggregated { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self) + write!( + f, + "PublicKeyAggregated {{ e3_id: {:?}, nodes: {}, pk_aggregation_proof: {} }}", + self.e3_id, + self.nodes.len(), + self.pk_aggregation_proof.is_some() + ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/publickey_aggregated.rs` around lines 22 - 29, The Display impl for PublicKeyAggregated currently uses write!(f, "{:?}", self) which will include pk_aggregation_proof and can leak/oversize logs; change the fmt implementation on PublicKeyAggregated to format all fields manually but redact or replace pk_aggregation_proof with a placeholder (e.g., "<redacted>" or its presence/len only). Update the fmt function for PublicKeyAggregated to reference pk_aggregation_proof by name and omit its payload while preserving other fields in the output.crates/multithread/src/multithread.rs (1)
264-326:⚠️ Potential issue | 🟠 MajorValidate committee invariants before deserialization/proving.
handle_pk_aggregation_proofcurrently trusts committee fields and share counts. Fail fast on malformed requests to avoid invalid C5 statements and hard-to-diagnose proof failures.Proposed fix
fn handle_pk_aggregation_proof( prover: &ZkProver, req: PkAggregationProofRequest, request: ComputeRequest, ) -> Result<ComputeResponse, ComputeRequestError> { + if req.keyshare_bytes.len() != req.committee_h + || req.committee_n == 0 + || req.committee_h == 0 + || req.committee_h > req.committee_n + || req.committee_threshold == 0 + || req.committee_threshold > req.committee_h + { + return Err(make_zk_error( + &request, + format!( + "invalid committee params: keyshares={}, n={}, h={}, threshold={}", + req.keyshare_bytes.len(), + req.committee_n, + req.committee_h, + req.committee_threshold + ), + )); + } + // 1. Build threshold BFV parameters from preset let (threshold_params, _dkg_params) = build_pair_for_preset(req.params_preset.clone()) .map_err(|e| make_zk_error(&request, format!("build_pair_for_preset: {}", e)))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 264 - 326, handle_pk_aggregation_proof currently proceeds without validating committee invariants; before any deserialization or proof generation, validate req.committee_n, req.committee_h, req.committee_threshold and req.keyshare_bytes length: ensure n > 0, h <= n, threshold <= h and threshold > 0, and req.keyshare_bytes.len() == n (and optionally fail if aggregated_pk_bytes empty). If any check fails, return an early ComputeRequestError (use the same error construction as existing map_err / make_zk_error patterns) so malformed requests fail fast; update validation near the top of handle_pk_aggregation_proof (before build_pair_for_preset/deserialization) and mention symbols committee_n, committee_h, committee_threshold, keyshare_bytes, PkAggregationCircuitData, and PublicKey usage locations to guide placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 154-160: The loop currently sets
PartyProofsToVerify.sender_party_id using the enumeration index (idx) which can
mis-associate proofs; instead extract the authoritative party ID from the
proof/event metadata (e.g., a field like proof.sender_party_id, proof.party_id,
or proof.metadata.party_id) and use that when constructing PartyProofsToVerify
in the c1_proofs iteration that fills party_proofs; ensure you handle the Option
case (Some(proof)) by reading the party id from the proof before
cloning/consuming it and propagate that mapping so sender_party_id is sourced
from the proof metadata rather than idx.
- Around line 363-376: The handler handle_compute_response currently processes
any ComputeResponse; add a guard that only proceeds if the response's e3_id
matches this aggregator's current e3 id (e.g., compare response.e3_id to
self.state.e3_id or the appropriate stored field); if they differ, return Ok(())
to ignore the event. Apply this check before matching on response.response so
c1/c5 handling (handle_c1_verification_response and handle_c5_proof_response)
only runs for responses belonging to this aggregator's e3 flow.
- Around line 303-310: The PkAggregation request is setting committee_threshold:
0 which produces an invalid C5 statement; instead compute and pass the real
threshold before constructing ZkRequest::PkAggregation — locate the
PkAggregationProofRequest construction and replace the hardcoded 0 with the
actual threshold derived from the preset/committee (e.g. resolve from
self.params_preset or from the committee_h/Committee type via its threshold
getter) so committee_threshold contains the real threshold value used for this
committee.
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 466-475: The current code unconditionally enables
PublicKeyAggregatorExtension via PublicKeyAggregatorExtension::create with a
hardcoded BfvPreset::InsecureThreshold512 and may run without a configured
prover, causing runtime C1/C5 failures; update the builder (where e3_builder is
composed) to (1) require/verify that a ZK prover/backend is configured before
calling PublicKeyAggregatorExtension::create (use the same config/field used to
register the prover), and (2) replace the fixed BfvPreset::InsecureThreshold512
with a preset passed through the builder API (add a builder field like
aggregator_preset and a setter method) so PublicKeyAggregatorExtension::create
uses the configured preset; also ensure ensure_multithread(&bus) still runs but
do not enable aggregation unless the prover and preset are valid.
In `@crates/events/src/enclave_event/publickey_aggregated.rs`:
- Around line 22-29: The Display impl for PublicKeyAggregated currently uses
write!(f, "{:?}", self) which will include pk_aggregation_proof and can
leak/oversize logs; change the fmt implementation on PublicKeyAggregated to
format all fields manually but redact or replace pk_aggregation_proof with a
placeholder (e.g., "<redacted>" or its presence/len only). Update the fmt
function for PublicKeyAggregated to reference pk_aggregation_proof by name and
omit its payload while preserving other fields in the output.
In `@crates/multithread/src/multithread.rs`:
- Around line 264-326: handle_pk_aggregation_proof currently proceeds without
validating committee invariants; before any deserialization or proof generation,
validate req.committee_n, req.committee_h, req.committee_threshold and
req.keyshare_bytes length: ensure n > 0, h <= n, threshold <= h and threshold >
0, and req.keyshare_bytes.len() == n (and optionally fail if aggregated_pk_bytes
empty). If any check fails, return an early ComputeRequestError (use the same
error construction as existing map_err / make_zk_error patterns) so malformed
requests fail fast; update validation near the top of
handle_pk_aggregation_proof (before build_pair_for_preset/deserialization) and
mention symbols committee_n, committee_h, committee_threshold, keyshare_bytes,
PkAggregationCircuitData, and PublicKey usage locations to guide placement.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/aggregator/Cargo.tomlcrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/entrypoint/src/start/aggregator_start.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rsexamples/CRISP/test/crisp.spec.tstemplates/default/enclave.config.yamltemplates/default/server/index.ts
💤 Files with no reviewable changes (1)
- templates/default/server/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/entrypoint/src/start/aggregator_start.rs
- examples/CRISP/test/crisp.spec.ts
- crates/events/src/enclave_event/compute_request/mod.rs
- crates/tests/tests/integration.rs
- crates/events/src/enclave_event/compute_request/zk.rs
- templates/default/enclave.config.yaml
cedoor
reviewed
Mar 3, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
466-475:⚠️ Potential issue | 🟠 MajorFail fast if
pubkey_aggis enabled without a ZK backend.This block enables C5 proof generation, but it still allows
self.zk_backendto beNone; in that caseensure_multithreadfalls back to non-ZK attach (Line 557), which can lead to missing proofs/runtime errors instead of a clear config error.Suggested hardening
if self.pubkey_agg { info!("Setting up PublicKeyAggregationExtension"); + if self.zk_backend.is_none() { + return Err(anyhow::anyhow!( + "ZK backend is required when public key aggregation is enabled" + )); + } // Ensure multithread worker is available for C1 verification and C5 proof generation let _ = self.ensure_multithread(&bus); // TODO: Make BfvPreset configurable via builder method. // Currently hardcoded to InsecureThreshold512 for C5 proof generation. // Production deployments should use the appropriate threshold preset. let aggregator_preset = DEFAULT_BFV_PRESET; e3_builder = e3_builder.with(PublicKeyAggregatorExtension::create( &bus, aggregator_preset, )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 466 - 475, The code currently adds PublicKeyAggregatorExtension (via PublicKeyAggregatorExtension::create and with DEFAULT_BFV_PRESET) even when self.zk_backend is None, which can lead to missing C5 proofs at runtime; update the builder to fail fast by checking self.zk_backend (or equivalent config flag) before calling ensure_multithread or PublicKeyAggregatorExtension::create and return a clear configuration error (or Err) if pubkey aggregation is enabled but self.zk_backend.is_none(); locate the check around ensure_multithread, e3_builder, and PublicKeyAggregatorExtension::create and add the validation (with a descriptive error message) so the builder refuses to construct the node when pubkey_agg is requested without a ZK backend.
🧹 Nitpick comments (2)
templates/default/deployed_contracts.json (1)
63-63: Harden reproducibility for ABI-encodedinitDataliterals.These long encoded payloads are correct-by-construction only when generated, not hand-maintained. Consider enforcing a generation/validation step in CI so
constructorArgsandinitDatacannot diverge silently.Also applies to: 85-85, 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/deployed_contracts.json` at line 63, The literal ABI-encoded initData in deployed_contracts.json (key "initData") can drift from its human-readable constructorArgs; add a generation/validation step to CI that regenerates initData from constructorArgs (or validates equality) during pre-merge checks and fails the build when they diverge, ensuring initData values (and other occurrences noted at lines ~85 and ~108) are produced programmatically from the ABI and constructor arguments rather than hand-edited.templates/default/enclave.config.yaml (1)
7-19: Consider adding a CI guard for config/snapshot drift.These values are easy to desynchronize over time. A lightweight CI check comparing this file’s
deploy_blockvalues againsttemplates/default/deployed_contracts.jsonwould catch stale updates early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/enclave.config.yaml` around lines 7 - 19, The YAML contains hard-coded deploy_block values (e.g., deploy_block under ciphernode_registry, bonding_registry, fee_token, e3_program) that can drift from the canonical source; add a CI guard that loads templates/default/deployed_contracts.json and compares each contract's deploy_block and address against the values in templates/default/enclave.config.yaml, failing the check (non-zero exit) when any deploy_block or address mismatches are found so PRs must update both files or the JSON; implement the check as a lightweight script (Node/Python/bash) invoked in CI that reports which keys (contract names) differ and a suggestion to sync the files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 466-475: The code currently adds PublicKeyAggregatorExtension (via
PublicKeyAggregatorExtension::create and with DEFAULT_BFV_PRESET) even when
self.zk_backend is None, which can lead to missing C5 proofs at runtime; update
the builder to fail fast by checking self.zk_backend (or equivalent config flag)
before calling ensure_multithread or PublicKeyAggregatorExtension::create and
return a clear configuration error (or Err) if pubkey aggregation is enabled but
self.zk_backend.is_none(); locate the check around ensure_multithread,
e3_builder, and PublicKeyAggregatorExtension::create and add the validation
(with a descriptive error message) so the builder refuses to construct the node
when pubkey_agg is requested without a ZK backend.
---
Nitpick comments:
In `@templates/default/deployed_contracts.json`:
- Line 63: The literal ABI-encoded initData in deployed_contracts.json (key
"initData") can drift from its human-readable constructorArgs; add a
generation/validation step to CI that regenerates initData from constructorArgs
(or validates equality) during pre-merge checks and fails the build when they
diverge, ensuring initData values (and other occurrences noted at lines ~85 and
~108) are produced programmatically from the ABI and constructor arguments
rather than hand-edited.
In `@templates/default/enclave.config.yaml`:
- Around line 7-19: The YAML contains hard-coded deploy_block values (e.g.,
deploy_block under ciphernode_registry, bonding_registry, fee_token, e3_program)
that can drift from the canonical source; add a CI guard that loads
templates/default/deployed_contracts.json and compares each contract's
deploy_block and address against the values in
templates/default/enclave.config.yaml, failing the check (non-zero exit) when
any deploy_block or address mismatches are found so PRs must update both files
or the JSON; implement the check as a lightweight script (Node/Python/bash)
invoked in CI that reports which keys (contract names) differ and a suggestion
to sync the files.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/ciphernode-builder/src/ciphernode_builder.rstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yaml
This was referenced Mar 22, 2026
This was referenced Mar 28, 2026
This was referenced May 18, 2026
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.
fix #1368
Summary by CodeRabbit
New Features
Chores