feat: make the ag part of the e3 committee [skip-line-limit]#1493
Closed
hmzakhalid wants to merge 5 commits into
Closed
feat: make the ag part of the e3 committee [skip-line-limit]#1493hmzakhalid wants to merge 5 commits into
hmzakhalid wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughReplaces a centralized aggregator with decentralized, rank-ordered aggregation: finalized-committee members perform public-key and plaintext aggregation with rank-based staggered submission and permissionless, proof-gated on-chain publication; sortition provides local rank and submission-rank queries; events and contracts updated to support this flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Ciphernode (local)
participant Sort as Sortition
participant Bus as Local Bus
participant EVM as Blockchain/Registry
Note over Node,Sort: DKG/public-key aggregation flow (high level)
Node->>Sort: GetLocalNodeSortitionRank(e3_id, seed, threshold)
Sort-->>Node: local_rank (Option)
alt is eligible
Node->>Bus: schedule CommitteeFinalizeRequested (with rank stagger)
Bus->>Node: CommitteeFinalized event
Node->>Sort: GetAggregatorSubmissionRank(e3_id, node)
Sort-->>Node: submission_rank (Option)
alt rank present
Node->>Node: delay = rank * INTERVAL
Note over Node: aggregate pk_shares, generate C5 proof
Node->>EVM: publishCommittee(e3_id, nodes, publicKey, proof, foldProof)
EVM-->>Node: tx result (accept / already-published)
end
else not eligible
Node-->>Node: remain inactive / buffer shares
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
hmzakhalid
changed the title
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
212-229:⚠️ Potential issue | 🔴 CriticalBind
nodesandpublicKeyto the finalized committee/proof before accepting the first publish.Right now a caller only needs a valid proof and a same-length
nodesarray. Because the first successful call wins, a mempool observer can front-run with the real proof but arbitrarynodesor arbitrarypublicKeybytes, permanently emitting inconsistentCommitteePublisheddata while advancing the stage. Please either validatenodesagainstc.topNodesand validate the suppliedpublicKeyagainst the verifier-derived commitment, or ignore the caller-provided metadata and emit the finalized committee data from storage instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 212 - 229, The code currently trusts caller-supplied nodes and publicKey when emitting CommitteePublished after verifying proof; instead, after decoding publicInputs to derive publicKeyHash and calling e3.pkVerifier.verify(proof, foldProof), bind the finalized committee metadata from storage: set c.publicKey = publicKeyHash and publicKeyHashes[e3Id] = publicKeyHash as you do, then call enclave.onCommitteePublished(e3Id, publicKeyHash) and emit CommitteePublished using c.topNodes and the derived publicKeyHash (not the caller-supplied nodes or publicKey). Reference symbols: nodes, publicKey, c.topNodes, publicInputs, publicKeyHash, c.publicKey, publicKeyHashes, e3.pkVerifier.verify, enclave.onCommitteePublished, emit CommitteePublished.crates/config/src/app_config.rs (1)
575-581:⚠️ Potential issue | 🔴 CriticalFix the YAML fixture indentation in the test.
pubkey_write_pathandplaintext_write_pathare indented as properties of the"two"list item, which is invalid YAML. They should be at the same indentation level asquic_portandpeersundernodes.ag.Proposed fix
nodes: ag: quic_port: 1235 peers: - "one" - "two" - pubkey_write_path: "./output/pubkey.bin" - plaintext_write_path: "./output/plaintext.txt" + pubkey_write_path: "./output/pubkey.bin" + plaintext_write_path: "./output/plaintext.txt"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config/src/app_config.rs` around lines 575 - 581, The YAML fixture under nodes.ag has mis-indented keys: pubkey_write_path and plaintext_write_path are mistakenly nested under the "two" list item; move pubkey_write_path and plaintext_write_path out of the peers list so they align with quic_port and peers under nodes.ag (update the fixture where nodes.ag, quic_port, peers, pubkey_write_path, and plaintext_write_path appear).
🧹 Nitpick comments (2)
deploy/copy-secrets.sh (1)
27-27: Quote array elements to prevent word splitting issues.Shellcheck SC2206: The array assignment should quote the variable expansions to prevent potential word splitting/globbing issues if any key contains special characters.
🔧 Proposed fix
-NET_KEYS=($NETWORK_KEY_CN1 $NETWORK_KEY_CN2 $NETWORK_KEY_CN3 $NETWORK_KEY_CN4) +NET_KEYS=("$NETWORK_KEY_CN1" "$NETWORK_KEY_CN2" "$NETWORK_KEY_CN3" "$NETWORK_KEY_CN4")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/copy-secrets.sh` at line 27, The NET_KEYS array assignment uses unquoted expansions which can cause word-splitting/globbing; update the NET_KEYS assignment so each expansion is quoted (e.g., use "$NETWORK_KEY_CN1" "$NETWORK_KEY_CN2" "$NETWORK_KEY_CN3" "$NETWORK_KEY_CN4") when building the NET_KEYS array to ensure keys with spaces or special characters are preserved; modify the NET_KEYS declaration accordingly.agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.md (1)
238-238: Consider adding a language specifier to the code fence.Static analysis flags this fenced code block as missing a language specifier. Adding one (e.g.,
```textor```plaintext) would satisfy linters and improve rendering in some Markdown viewers.📝 Suggested fix
-``` +```text CommitteeFinalizer actor receives CommitteeRequested event on each selected node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.md` at line 238, Add a language specifier to the fenced code block containing the text "CommitteeFinalizer actor receives CommitteeRequested event on each selected node" (replace the opening ``` with something like ```text or ```plaintext) so static linters and markdown renderers recognize the block language; update the single backtick fence at that location accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 637-639: Remove the leading space so the "## Phase 5: Plaintext
Aggregation (Eligible Finalized Committee Members, with C6 Verification & C7
Proof)" line is a proper markdown heading (not an indented code block) and
change the following fence from ``` to a labeled fence like ```text (or remove
the fence if not needed) so the section renders correctly and the markdownlint
warning is resolved.
- Around line 503-517: The flow diagram and text must be updated to the new
publishCommittee ABI: replace calls from publishCommittee(e3_id, nodes,
publicKey, pkHash) to publishCommittee(e3Id, nodes, proof, foldProof) (i.e.,
remove publicKey and pkHash from the call site in CiphernodeRegistrySolWriter
and the PublicKeyAggregated path), and update the on-chain sequence to show the
contract verifies the provided proof/foldProof, derives the pkHash internally,
sets publicKeyHashes[e3Id] = derivedPkHash, and then calls
enclave.onCommitteePublished(e3Id, derivedPkHash); also ensure any mention of
avoiding duplicate submission references publicKeyHashes[e3Id] and the derived
pkHash logic rather than an externally supplied pkHash.
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 668-694: The code treats a single GetFinalizedCommittee miss as
terminal, clearing pending_shares and stopping the actor; instead keep the actor
in the pending state and retry resolving the committee: when the spawned task
returns Ok(None) or Err(_), do not clear act.pending_shares or call ctx.stop();
either leave act.duty as PendingCommittee (or set it to PendingCommittee) and
schedule a retry via ctx.run_later or re-send the GetFinalizedCommittee query
(using the same sortition clone/e3_id used in the ctx.spawn call), or subscribe
to the finalization event to call resolve_aggregation_duty later; ensure
resolve_aggregation_duty and the AggregationDuty::Eligible branch still flush
pending_shares only after a successful committee resolution.
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 751-754: The CommitteeFinalizer is being attached inside
setup_evm_system for each enabled chain which causes multiple subscribers on the
shared bus and duplicate CommitteeFinalizeRequested events; move the
CommitteeFinalizer::attach(&bus, sortition.clone()) out of setup_evm_system so
it runs once during node startup (e.g., alongside other global component
attachments), ensure the attachment is executed only when pubkey_agg is true and
only once (use a single central startup/init path or a one-time guard) and keep
using the same bus and sortition instances so the finalizer subscribes exactly
once.
In `@crates/evm/src/enclave_sol_writer.rs`:
- Around line 236-250: The current logic treats "CiphertextOutputNotPublished"
like idempotent cases and logs it as resolved; instead, keep it fatal so
post-retry failures are propagated: in the block using format_evm_error
(variable decoded) inside the plaintext publication path (after
send_tx_with_retry()), change the conditional that currently checks
decoded.contains("InvalidStage") || decoded.contains("CommitteeDutiesCompleted")
|| decoded.contains("CiphertextOutputNotPublished") to only check for
"InvalidStage" and "CommitteeDutiesCompleted"; ensure any decoded string
containing "CiphertextOutputNotPublished" falls through to the
bus.err(EType::Evm, anyhow::anyhow!(...)) path so the error is not swallowed.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 460-470: The getter getCommitteeNodes currently reverts if
c.publicKey == bytes32(0), preventing reads when a committee is finalized but
not yet published; change the access check to allow reads when the committee is
finalized as well (e.g. replace the require(c.publicKey != bytes32(0),
CommitteeNotPublished()) with a condition that accepts c.publicKey != bytes32(0)
|| c.finalized) so getCommitteeNodes (which reads committees[e3Id] into
Committee c and returns c.topNodes and c.scoreOf) will return finalized (nodes,
scores) for a restarted writer; ensure you reference the actual finalized flag
name used in Committee (e.g. finalized or isFinalized) when implementing this
change.
- Around line 328-330: The require in the finalize path uses >= on
c.committeeDeadline which allows finalize and submitTicket() to both succeed at
the same timestamp; change the comparison to > (i.e., require(block.timestamp >
c.committeeDeadline, SubmissionWindowNotClosed())) to keep finalization
exclusive, or alternatively make submitTicket() enforce block.timestamp <
c.committeeDeadline so submissions are exclusive—update whichever path you
choose (the require using c.committeeDeadline or the submitTicket() timestamp
check) so the boundary is exclusive and the race is eliminated.
In `@packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol`:
- Around line 59-64: The current MockCiphernodeRegistry hardcodes a finalized
state and non-zero publicKeyHashes (publicKeyHashes and the Finalized status in
MockCiphernodeRegistry and MockCiphernodeRegistryEmptyKey), which prevents
should_finalize_committee() from exercising the pre-publication path; change the
mocks so the publication stage and hash are configurable (e.g., add a
setStage/setPublicKeyHash or constructor parameters) or alter the default in
MockCiphernodeRegistry to the pre-publication state (Finalized=false and
publicKeyHashes returning bytes32(0) for ordinary ids), and update the other
occurrences noted (lines ~124-128 and ~282-286) to use the same
configurable/default behavior so tests can toggle between published and
unpublished states.
In `@tests/integration/fns.sh`:
- Around line 146-148: The selection uses best_addr from paired addrs and scores
produced by getCommitteeNodes(), but sort -g treats values as floating-point and
can collapse distinct uint256 scores; change the pipeline to use an
integer-preserving numeric sort (use sort with the numeric (-n) field option on
column 2, e.g., sort -t$'\t' -k2,2 -n) so the scores array is compared as
integers and the correct primary node is chosen.
---
Outside diff comments:
In `@crates/config/src/app_config.rs`:
- Around line 575-581: The YAML fixture under nodes.ag has mis-indented keys:
pubkey_write_path and plaintext_write_path are mistakenly nested under the "two"
list item; move pubkey_write_path and plaintext_write_path out of the peers list
so they align with quic_port and peers under nodes.ag (update the fixture where
nodes.ag, quic_port, peers, pubkey_write_path, and plaintext_write_path appear).
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 212-229: The code currently trusts caller-supplied nodes and
publicKey when emitting CommitteePublished after verifying proof; instead, after
decoding publicInputs to derive publicKeyHash and calling
e3.pkVerifier.verify(proof, foldProof), bind the finalized committee metadata
from storage: set c.publicKey = publicKeyHash and publicKeyHashes[e3Id] =
publicKeyHash as you do, then call enclave.onCommitteePublished(e3Id,
publicKeyHash) and emit CommitteePublished using c.topNodes and the derived
publicKeyHash (not the caller-supplied nodes or publicKey). Reference symbols:
nodes, publicKey, c.topNodes, publicInputs, publicKeyHash, c.publicKey,
publicKeyHashes, e3.pkVerifier.verify, enclave.onCommitteePublished, emit
CommitteePublished.
---
Nitpick comments:
In `@agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.md`:
- Line 238: Add a language specifier to the fenced code block containing the
text "CommitteeFinalizer actor receives CommitteeRequested event on each
selected node" (replace the opening ``` with something like ```text or
```plaintext) so static linters and markdown renderers recognize the block
language; update the single backtick fence at that location accordingly.
In `@deploy/copy-secrets.sh`:
- Line 27: The NET_KEYS array assignment uses unquoted expansions which can
cause word-splitting/globbing; update the NET_KEYS assignment so each expansion
is quoted (e.g., use "$NETWORK_KEY_CN1" "$NETWORK_KEY_CN2" "$NETWORK_KEY_CN3"
"$NETWORK_KEY_CN4") when building the NET_KEYS array to ensure keys with spaces
or special characters are preserved; modify the NET_KEYS declaration
accordingly.
🪄 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: c3fbdff2-f9b6-48b6-ae02-673f65420693
📒 Files selected for processing (64)
agent/flow-trace/00_INDEX.mdagent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdagent/flow-trace/05_FAILURE_REFUND_SLASHING.mdcrates/README.mdcrates/aggregator/src/committee_finalizer.rscrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/cli/src/start.rscrates/config/src/app_config.rscrates/entrypoint/src/start/mod.rscrates/entrypoint/src/start/start.rscrates/events/src/committee.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rscrates/sortition/Readme.mdcrates/sortition/src/ciphernode_selector.rscrates/sortition/src/sortition.rsdappnode/README.mddappnode/config.template.yamldappnode/docker-compose.ymldappnode/entrypoint.shdappnode/setup-wizard.ymldeploy/agg.yamldeploy/cn2.yamldeploy/cn3.yamldeploy/cn4.yamldeploy/copy-secrets.shdeploy/docker-compose.ymldeploy/inspect.shdeploy/local/nodes.shdeploy/local/start.shdeploy/swarm_deployment.mddocs/pages/CRISP/introduction.mdxdocs/pages/ciphernode-operators/running.mdxdocs/pages/ciphernode-operators/tickets-and-sortition.mdxdocs/pages/project-template.mdxexamples/CRISP/Readme.mdexamples/CRISP/enclave.config.yamlexamples/CRISP/scripts/dev_cipher.shexamples/CRISP/scripts/setup_testnet.shpackages/enclave-contracts/README.mdpackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/hardhat.config.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tstemplates/default/enclave.config.yamltemplates/default/scripts/dev_ciphernodes.shtests/integration/base.shtests/integration/enclave.config.yamltests/integration/fns.shtests/integration/persist.sh
💤 Files with no reviewable changes (11)
- dappnode/docker-compose.yml
- crates/entrypoint/src/start/mod.rs
- examples/CRISP/enclave.config.yaml
- examples/CRISP/scripts/dev_cipher.sh
- templates/default/scripts/dev_ciphernodes.sh
- deploy/local/nodes.sh
- dappnode/config.template.yaml
- templates/default/enclave.config.yaml
- dappnode/setup-wizard.yml
- deploy/agg.yaml
- dappnode/entrypoint.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
deploy/local/start.sh (1)
82-88:⚠️ Potential issue | 🟠 MajorAdd CN4 and CN5 to the ciphernode registry in the
deploy_contracts()function.Wallets for
cn4andcn5are configured in line 103, but the registry additions only includeCN1,CN2, andCN3. To fully set up the local development environment, add the following lines after line 88:CN4=0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65 CN5=0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc pnpm ciphernode:add --ciphernode-address "$CN4" --network "localhost" pnpm ciphernode:add --ciphernode-address "$CN5" --network "localhost"This aligns the registry additions with the wallet setup and matches the pattern used in other deployment scripts (e.g.,
deploy/local/contracts.sh,examples/CRISP/scripts/dev_cipher.sh).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/local/start.sh` around lines 82 - 88, The deploy_contracts() block only registers CN1–CN3; add CN4 and CN5 variables and corresponding registry commands to match the wallet setup: define CN4=0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65 and CN5=0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc immediately after the existing CN3 definition, then call pnpm ciphernode:add --ciphernode-address "$CN4" --network "localhost" and pnpm ciphernode:add --ciphernode-address "$CN5" --network "localhost" (same pattern as the existing pnpm ciphernode:add lines) so the registry additions align with the configured wallets.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (2)
198-229:⚠️ Potential issue | 🔴 CriticalValidate
nodesandpublicKeybefore emitting a permissionless publish.After dropping
onlyOwner, both fields are attacker-controlled calldata. A frontrunner can reuse a valid C5 proof but publish a bogus node list and/or boguspublicKeybytes unless this call also bindsnodestoc.topNodesandpublicKeyto the proof-derived commitment. That can poisonCommitteePublishedfor off-chain consumers even though the proof itself is valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 198 - 229, The publishCommittee function must validate attacker-controlled calldata before emitting: add checks that the supplied nodes array exactly matches stored c.topNodes (iterate and compare each address after the existing length check) and that the supplied publicKey bytes bind to the proof-derived publicKeyHash (verify bytes32(public key hash) by comparing publicKeyHash == bytes32(keccak256(publicKey)) or the correct encoding used by the proof, then require they match) before setting c.publicKey, publicKeyHashes[e3Id], calling enclave.onCommitteePublished, or emitting CommitteePublished.
318-330:⚠️ Potential issue | 🟠 MajorKeep ticket submission and finalization windows non-overlapping.
finalizeCommitteenow allowsblock.timestamp >= committeeDeadline, butsubmitTicketstill acceptsblock.timestamp <= committeeDeadline. At the exact deadline both transactions are valid, so same-block ordering can finalize/fail the committee before last-moment ticket submissions land.One minimal fix
- require( - block.timestamp >= c.committeeDeadline, - SubmissionWindowNotClosed() - ); + require( + block.timestamp > c.committeeDeadline, + SubmissionWindowNotClosed() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 318 - 330, The two functions allow both submitTicket and finalizeCommittee to be valid at block.timestamp == committeeDeadline; make the windows non-overlapping by changing the time check in submitTicket to require(block.timestamp < c.committeeDeadline, SubmissionWindowClosed()) instead of <=, so ticket submissions stop strictly before committeeDeadline (leave finalizeCommittee's check as >=); update any tests or comments referencing the inclusive deadline.crates/config/src/app_config.rs (1)
575-581:⚠️ Potential issue | 🔴 CriticalFix the YAML fixture indentation under
nodes.ag.
pubkey_write_pathandplaintext_write_pathare nested under the secondpeersitem instead of at the node level, causingserde_yaml::from_str(config_str)to fail during test execution.Suggested fix
ag: quic_port: 1235 peers: - "one" - "two" - pubkey_write_path: "./output/pubkey.bin" - plaintext_write_path: "./output/plaintext.txt" + pubkey_write_path: "./output/pubkey.bin" + plaintext_write_path: "./output/plaintext.txt"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config/src/app_config.rs` around lines 575 - 581, The YAML fixture under nodes.ag has incorrect indentation causing pubkey_write_path and plaintext_write_path to be children of the second peers entry; fix the fixture so pubkey_write_path and plaintext_write_path are aligned at the same indentation level as quic_port and peers under nodes.ag (so they are node-level fields, not under a peers item) so that serde_yaml::from_str(config_str) can parse into the AppConfig/Node structs correctly.
🧹 Nitpick comments (3)
dappnode/README.md (1)
161-163: Clarify which operations requirePRIVATE_KEYin practice.This is a good update, but it would help operators to add 1–2 concrete examples (e.g., committee publish/finalization submissions) so they can decide when leaving it unset is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dappnode/README.md` around lines 161 - 163, Update the PRIVATE_KEY documentation to list concrete operations that require an on-chain signing key: state that PRIVATE_KEY (used by entrypoint.sh to run `enclave wallet set --config /data/config.yaml --private-key "$PRIVATE_KEY"`) is required when the node must submit transactions such as committee publish actions and finalization/consensus submissions, but can be left unset for read-only duties like syncing, indexing, or serving RPCs; add 1–2 short examples like "committee publish (attest/post results to chain)" and "finalization/validator submissions" so operators can decide whether to provide PRIVATE_KEY.examples/CRISP/scripts/setup_testnet.sh (1)
57-58: Optional: align env var naming with the new deployer terminology.Comments now refer to a deployer/admin key, but the script still relies on
PRIVATE_KEY_AG. Consider introducing a deployer-named variable with fallback to preserve backward compatibility.♻️ Suggested compatibility-safe tweak
- export PRIVATE_KEY="$PRIVATE_KEY_AG" + export PRIVATE_KEY="${PRIVATE_KEY_DEPLOYER:-$PRIVATE_KEY_AG}" ... - export PRIVATE_KEY="$PRIVATE_KEY_AG" + export PRIVATE_KEY="${PRIVATE_KEY_DEPLOYER:-$PRIVATE_KEY_AG}"Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/scripts/setup_testnet.sh` around lines 57 - 58, Update the script to introduce a deployer-named env var (e.g. DEPLOYER_PRIVATE_KEY or PRIVATE_KEY_DEPLOYER) while preserving backward compatibility with the existing PRIVATE_KEY_AG: set PRIVATE_KEY from the new deployer var if present, otherwise fall back to PRIVATE_KEY_AG (replace the current export PRIVATE_KEY="$PRIVATE_KEY_AG" with this fallback logic), and apply the same change to the other occurrence mentioned (the block around the second export at lines 81-82) so comments and variable names consistently use "deployer" terminology without breaking existing setups.packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol (1)
124-128: Consider making committee stage configurable in these mocks.Both helpers now always return
Finalized, but the new registry writer branches on the current stage before callingfinalizeCommittee/publishCommittee. Without aRequestedpath here, these mocks can't exercise that new gating in tests.Also applies to: 282-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol` around lines 124 - 128, The mock currently hardcodes Finalized in MockCiphernodeRegistry.getCommitteeStage which prevents exercising the Requested path; change the mock to store a mutable CommitteeStage state (e.g., a private ICiphernodeRegistry.CommitteeStage variable with a public setter like setCommitteeStage) and return that variable from getCommitteeStage (defaulting to Finalized to preserve existing tests); apply the same pattern to the other identical helper (the one around lines 282-286) so tests can toggle between Requested and Finalized before calling finalizeCommittee/publishCommittee.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.md`:
- Line 238: The fenced code block containing "CommitteeFinalizer actor receives
CommitteeRequested event on each selected node" is missing a language
identifier, causing markdownlint MD040; update that fenced block (the
triple-backtick block surrounding that line) to include a language token such as
```text so it becomes ```text followed by the line and a closing ``` to satisfy
the linter.
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 503-507: Update the diagram and example calls to match the current
contract ABIs: locate the CiphernodeRegistrySolWriter section and replace the
old signatures publishCommittee(e3_id, nodes, publicKey, pkHash) and
publishPlaintextOutput(e3Id, output, proof) with the new signatures that include
the proof/fold-proof payloads (e.g., the publishCommittee variant that accepts
the aggregated public key plus proof/foldProof fields and the
publishPlaintextOutput variant that submits both output and its fold-proof).
Ensure references to parameters publicKey, pkHash, and proof are updated to the
new parameter names used by the ABI (foldProof/proofPayload as applicable) and
make the same updates at the other occurrence around lines 702-706.
- Around line 456-458: Remove the stray code-fence that indented the "Phase 2:
Public Key Aggregation (Eligible Finalized Committee Members, with C5 Proof)"
heading and similarly fix the "Phase 5" section; convert the diagram fences from
plain backticks into normal markdown headings and use fenced code blocks labeled
with a language of `text` (i.e., replace the leading/closing ``` with ```text)
for the flow-diagram contents so the headings are not inside a code block and
the diagrams are fenced as ```text blocks; target the sections titled "Phase 2:
Public Key Aggregation..." and "Phase 5" (and the diagram blocks around lines
referenced 637-639) when making the changes.
In `@crates/aggregator/src/committee_finalizer.rs`:
- Around line 108-146: The handler that creates the finalization timer currently
aborts if GetLocalNodeSortitionRank (sent via sortition.send) fails or returns
None, so add fallback/reschedule logic: in the CommitteeRequested handler, when
sortition.send(local_rank_request) returns Err or None, do not return None
immediately — instead enqueue a retry (e.g., spawn an async retry with a short
backoff) that re-attempts GetLocalNodeSortitionRank up to a bounded number of
attempts or until success, and only then create the finalization timer;
alternatively, if you prefer immediate progress, allow creating the timer with a
safe default rank and mark the E3 as “pending rank” and subscribe to
node-registry events to patch the rank later (use the same get_node_index flow
used by E3Requested). Ensure you update the local symbols referenced
(GetLocalNodeSortitionRank, sortition.send, local_rank, CommitteeRequested
handler and any timer creation logic) so the code never silently drops
finalization when the lookup is temporarily unavailable.
In `@crates/aggregator/src/ext.rs`:
- Around line 118-121: The hydrate() path currently treats a missing META_KEY as
fatal by using ctx.get_dependency(META_KEY).ok_or_else(...), which can abort
rehydration of an existing PublicKeyAggregator; change this to mirror the
FHE_KEY branch: query ctx.get_dependency(META_KEY) and, if absent, log a warning
and leave the associated recipient/threshold unset (or use an Option/default)
instead of returning an error. Update the construction of PublicKeyAggregator to
accept the optional metadata (e.g., threshold_m) and ensure node_address usage
remains unchanged, so hydrate() continues when META_KEY is missing.
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 683-691: The actor currently clears act.pending_shares and calls
ctx.stop() when GetFinalizedCommittee returns Ok(None) in PendingCommittee,
which loses early shares; instead, remove the act.pending_shares.clear() and
ctx.stop() on Ok(None), keep the actor in PendingCommittee (or set a pending
flag) and ensure it remains subscribed/waits for CommitteeFinalized; implement
or wire a handler for CommitteeFinalized that re-resolves the committee and
transitions to the aggregation path used by CiphertextOutputPublished (reusing
existing aggregation logic), so buffered shares are retained and processed once
GetFinalizedCommittee returns Some; keep references to PendingCommittee,
pending_shares, GetFinalizedCommittee, CommitteeFinalized,
CiphertextOutputPublished, and the places that previously called ctx.stop() to
guide changes.
In `@crates/events/src/committee.rs`:
- Around line 78-87: The aggregator_count currently uses
self.len().saturating_sub(threshold_m) which yields 0 when threshold_m >=
self.len(), causing no aggregators for valid M==N cases; change aggregator_count
to ensure at least one eligible aggregator (e.g., return the saturating_sub
result but then .max(1) or otherwise clamp to a minimum of 1) so
aggregation_rank_for and related logic can assign a duty; update the function
aggregator_count(&self, threshold_m: usize) accordingly and ensure
aggregation_rank_for(&self, addr: &str, threshold_m: usize) continues to rely on
it.
In `@crates/evm/src/ciphernode_registry_sol.rs`:
- Around line 427-439: The preflight RPC checks (e.g.,
should_finalize_committee, getCommitteeStage, publicKeyHashes) are currently
aborting the handler on any error or non-finalized result, which can drop the
only finalize/publish attempt; change them to be advisory: only return early
when the read conclusively shows the committee is already finalized/published
(e.g., Ok(false meaning already finalized/published), or explicit published
state), but on Err(_) or non-finalized results fall through to the transaction
path (or retry the read) so
send_tx_with_retry("finalizeCommittee"/"publishCommittee", ...) still runs;
update the match/if branches in should_finalize_committee handling and the
getCommitteeStage/publicKeyHashes checks (including the logic around the
finalizeCommittee submission and publishCommittee path referenced) to log errors
instead of returning and to proceed to send_tx_with_retry when state is not
conclusively already-done.
In `@crates/sortition/Readme.md`:
- Line 117: Update the sequence diagram callout to use the current contract
method name by replacing publishPublicKey(...) with publishCommittee(...); also
verify and align the callout's parameter list (e.g., e3Id, pubkey, nodes) to
match the contract-facing docs/tests for publishCommittee so the diagram and
contract API are consistent (look for the diagram line containing
"PublicKeyAggregator->>CiphernodeRegistry" and change its method label and
params accordingly).
In `@deploy/copy-secrets.sh`:
- Around line 20-27: The script currently maps keys by position (TARGETS and
NET_KEYS) so skipping a target shifts indices and assigns the wrong key; change
to an explicit stable mapping: create an associative array (e.g., KEY_MAP) that
maps each target name (elements of TARGETS) to its corresponding network key
variable (NETWORK_KEY_CN1, etc.), then in the copy loop look up the key with
KEY_MAP["$target"] rather than using a positional index into NET_KEYS; update
any references to NET_KEYS and the loop that checks target existence to use this
lookup so skipping a target won’t change which key is used for other targets.
In `@tests/integration/fns.sh`:
- Around line 159-174: The SC2155 warning comes from declaring and assigning in
one local statement in waiton_any_pubkey; update the function to declare
variables separately (e.g., use local timeout="${1:-1300}" then local start_time
on its own line, then assign start_time=$(date +%s)), and keep the loop and use
of ciphernode_pubkey_path unchanged so only the local declarations are split to
satisfy shellcheck.
- Around line 123-152: The get_primary_committee_node function uses combined
local declarations with command substitutions (e.g., local start_time=$(date
+%s), local raw=$(...), local -a addrs scores, best_addr=$(...)) which triggers
shellcheck SC2155 and can mask return codes; refactor by first declaring
variables with local (e.g., local start_time raw best_addr addrs scores) and
then assign them on separate lines (start_time=$(date +%s); raw=$(cast call
...); populate addrs/scores with mapfile assignments; best_addr=$(paste ...)) so
assignments are separate from declarations and failures aren’t masked while
preserving the existing logic in get_primary_committee_node.
---
Outside diff comments:
In `@crates/config/src/app_config.rs`:
- Around line 575-581: The YAML fixture under nodes.ag has incorrect indentation
causing pubkey_write_path and plaintext_write_path to be children of the second
peers entry; fix the fixture so pubkey_write_path and plaintext_write_path are
aligned at the same indentation level as quic_port and peers under nodes.ag (so
they are node-level fields, not under a peers item) so that
serde_yaml::from_str(config_str) can parse into the AppConfig/Node structs
correctly.
In `@deploy/local/start.sh`:
- Around line 82-88: The deploy_contracts() block only registers CN1–CN3; add
CN4 and CN5 variables and corresponding registry commands to match the wallet
setup: define CN4=0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65 and
CN5=0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc immediately after the existing
CN3 definition, then call pnpm ciphernode:add --ciphernode-address "$CN4"
--network "localhost" and pnpm ciphernode:add --ciphernode-address "$CN5"
--network "localhost" (same pattern as the existing pnpm ciphernode:add lines)
so the registry additions align with the configured wallets.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 198-229: The publishCommittee function must validate
attacker-controlled calldata before emitting: add checks that the supplied nodes
array exactly matches stored c.topNodes (iterate and compare each address after
the existing length check) and that the supplied publicKey bytes bind to the
proof-derived publicKeyHash (verify bytes32(public key hash) by comparing
publicKeyHash == bytes32(keccak256(publicKey)) or the correct encoding used by
the proof, then require they match) before setting c.publicKey,
publicKeyHashes[e3Id], calling enclave.onCommitteePublished, or emitting
CommitteePublished.
- Around line 318-330: The two functions allow both submitTicket and
finalizeCommittee to be valid at block.timestamp == committeeDeadline; make the
windows non-overlapping by changing the time check in submitTicket to
require(block.timestamp < c.committeeDeadline, SubmissionWindowClosed()) instead
of <=, so ticket submissions stop strictly before committeeDeadline (leave
finalizeCommittee's check as >=); update any tests or comments referencing the
inclusive deadline.
---
Nitpick comments:
In `@dappnode/README.md`:
- Around line 161-163: Update the PRIVATE_KEY documentation to list concrete
operations that require an on-chain signing key: state that PRIVATE_KEY (used by
entrypoint.sh to run `enclave wallet set --config /data/config.yaml
--private-key "$PRIVATE_KEY"`) is required when the node must submit
transactions such as committee publish actions and finalization/consensus
submissions, but can be left unset for read-only duties like syncing, indexing,
or serving RPCs; add 1–2 short examples like "committee publish (attest/post
results to chain)" and "finalization/validator submissions" so operators can
decide whether to provide PRIVATE_KEY.
In `@examples/CRISP/scripts/setup_testnet.sh`:
- Around line 57-58: Update the script to introduce a deployer-named env var
(e.g. DEPLOYER_PRIVATE_KEY or PRIVATE_KEY_DEPLOYER) while preserving backward
compatibility with the existing PRIVATE_KEY_AG: set PRIVATE_KEY from the new
deployer var if present, otherwise fall back to PRIVATE_KEY_AG (replace the
current export PRIVATE_KEY="$PRIVATE_KEY_AG" with this fallback logic), and
apply the same change to the other occurrence mentioned (the block around the
second export at lines 81-82) so comments and variable names consistently use
"deployer" terminology without breaking existing setups.
In `@packages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol`:
- Around line 124-128: The mock currently hardcodes Finalized in
MockCiphernodeRegistry.getCommitteeStage which prevents exercising the Requested
path; change the mock to store a mutable CommitteeStage state (e.g., a private
ICiphernodeRegistry.CommitteeStage variable with a public setter like
setCommitteeStage) and return that variable from getCommitteeStage (defaulting
to Finalized to preserve existing tests); apply the same pattern to the other
identical helper (the one around lines 282-286) so tests can toggle between
Requested and Finalized before calling finalizeCommittee/publishCommittee.
🪄 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: 05b99f2a-000b-424b-8dd3-6ebf89de279b
📒 Files selected for processing (65)
agent/flow-trace/00_INDEX.mdagent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdagent/flow-trace/05_FAILURE_REFUND_SLASHING.mdcrates/README.mdcrates/aggregator/src/committee_finalizer.rscrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/cli/src/start.rscrates/config/src/app_config.rscrates/entrypoint/src/start/mod.rscrates/entrypoint/src/start/start.rscrates/events/src/committee.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rscrates/keyshare/src/ext.rscrates/sortition/Readme.mdcrates/sortition/src/ciphernode_selector.rscrates/sortition/src/sortition.rsdappnode/README.mddappnode/config.template.yamldappnode/docker-compose.ymldappnode/entrypoint.shdappnode/setup-wizard.ymldeploy/agg.yamldeploy/cn2.yamldeploy/cn3.yamldeploy/cn4.yamldeploy/copy-secrets.shdeploy/docker-compose.ymldeploy/inspect.shdeploy/local/nodes.shdeploy/local/start.shdeploy/swarm_deployment.mddocs/pages/CRISP/introduction.mdxdocs/pages/ciphernode-operators/running.mdxdocs/pages/ciphernode-operators/tickets-and-sortition.mdxdocs/pages/project-template.mdxexamples/CRISP/Readme.mdexamples/CRISP/enclave.config.yamlexamples/CRISP/scripts/dev_cipher.shexamples/CRISP/scripts/setup_testnet.shpackages/enclave-contracts/README.mdpackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/hardhat.config.tspackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tstemplates/default/enclave.config.yamltemplates/default/scripts/dev_ciphernodes.shtests/integration/base.shtests/integration/enclave.config.yamltests/integration/fns.shtests/integration/persist.sh
💤 Files with no reviewable changes (11)
- dappnode/docker-compose.yml
- deploy/agg.yaml
- examples/CRISP/scripts/dev_cipher.sh
- dappnode/setup-wizard.yml
- dappnode/config.template.yaml
- crates/entrypoint/src/start/mod.rs
- templates/default/scripts/dev_ciphernodes.sh
- templates/default/enclave.config.yaml
- dappnode/entrypoint.sh
- deploy/local/nodes.sh
- examples/CRISP/enclave.config.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
agent/flow-trace/04_DKG_AND_COMPUTATION.md (2)
702-716:⚠️ Potential issue | 🟡 MinorStill unresolved: the plaintext publication trace shows the old ABI.
The runtime writer now submits
publishPlaintextOutput(e3Id, output, proof, foldProof), but this section still documents the 3-argument form and a verification path that only mentionsproof. That leaves the flow trace out of sync with the current implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md` around lines 702 - 716, The flow trace documents an outdated ABI for publishPlaintextOutput; update the EnclaveSolWriter section and the on-chain pseudocode to match the current 4-argument signature publishPlaintextOutput(e3Id, output, proof, foldProof) and the corresponding verification logic. Replace the 3-arg lines and the single decryptionVerifier.verify call with the new call signature and show both proof parameters (e.g., decryptionVerifier.verify(e3Id, keccak256(output), proof, foldProof) or the actual verifier function(s) used), and ensure any mentions of only “proof” are updated to include foldProof and reflect the real verification steps used by the current implementation.
456-458:⚠️ Potential issue | 🟡 MinorStill unresolved: fix the heading/fence rendering in these updated phases.
Line 456 is still indented into a code block, and the fences at Lines 458 and 639 are still unlabeled, so these sections render incorrectly and keep the markdownlint warnings alive.
Also applies to: 637-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md` around lines 456 - 458, The "## Phase 2: Public Key Aggregation (Eligible Finalized Committee Members, with C5 Proof)" heading is being rendered inside a code fence because it is indented; remove the leading indentation so the heading starts at column 0 and is not inside a code block, and replace unlabeled triple-backtick fences (the ones around the heading and at the later fence near the block ending at line 639) with properly labeled fences (e.g., ```text or ```md) and ensure each opening ``` has a matching closing ``` so the section and subsequent phases render correctly; specifically edit the heading line and the two unlabeled fences surrounding it to be non-indented and to use labeled fenced code blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 702-716: The flow trace documents an outdated ABI for
publishPlaintextOutput; update the EnclaveSolWriter section and the on-chain
pseudocode to match the current 4-argument signature
publishPlaintextOutput(e3Id, output, proof, foldProof) and the corresponding
verification logic. Replace the 3-arg lines and the single
decryptionVerifier.verify call with the new call signature and show both proof
parameters (e.g., decryptionVerifier.verify(e3Id, keccak256(output), proof,
foldProof) or the actual verifier function(s) used), and ensure any mentions of
only “proof” are updated to include foldProof and reflect the real verification
steps used by the current implementation.
- Around line 456-458: The "## Phase 2: Public Key Aggregation (Eligible
Finalized Committee Members, with C5 Proof)" heading is being rendered inside a
code fence because it is indented; remove the leading indentation so the heading
starts at column 0 and is not inside a code block, and replace unlabeled
triple-backtick fences (the ones around the heading and at the later fence near
the block ending at line 639) with properly labeled fences (e.g., ```text or
```md) and ensure each opening ``` has a matching closing ``` so the section and
subsequent phases render correctly; specifically edit the heading line and the
two unlabeled fences surrounding it to be non-indented and to use labeled fenced
code blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 237ea2ff-ce79-4008-b501-2d8367b9f308
📒 Files selected for processing (4)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/ciphernode-builder/src/ciphernode_builder.rscrates/evm/src/enclave_sol_writer.rstests/integration/fns.sh
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/sortition/src/ciphernode_selector.rs (1)
211-243:⚠️ Potential issue | 🟠 MajorMove the
selected_e3sinsert after the publish succeeds.If
bus.publish(CiphernodeSelected { ... })fails, this E3 is still marked as selected and every laterAggregatorSelectedfor the same E3 returns early. That can suppress the onlyCiphernodeSelectedemission untilE3RequestCompleteclears the set.Suggested change
- self.selected_e3s.insert(msg.e3_id.clone()); - info!( node = self.address, party_id = party_id, "Node is in finalized committee, emitting CiphernodeSelected" ); @@ bus.publish( CiphernodeSelected { party_id: party_id as u64, e3_id: msg.e3_id, threshold_m: e3_meta.threshold_m, @@ }, ec, )?; + + self.selected_e3s.insert(msg.e3_id.clone());Also applies to: 258-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sortition/src/ciphernode_selector.rs` around lines 211 - 243, Currently the code inserts into selected_e3s before publishing the CiphernodeSelected event, so if bus.publish(CiphernodeSelected { ... }) fails the E3 remains marked and subsequent AggregatorSelected handlers return early; move the self.selected_e3s.insert(msg.e3_id.clone()) call to after a successful publish (i.e., after bus.publish(...) completes without error) in the CiphernodeSelector handling path, and apply the same fix to the other occurrence referenced around the 258-270 block; ensure insertion only happens when publish succeeded and that E3RequestComplete still clears selected_e3s as before.
♻️ Duplicate comments (3)
crates/evm/src/enclave_sol_writer.rs (1)
175-183:⚠️ Potential issue | 🟠 MajorDon't let preflight rank/stage checks consume this node's only plaintext attempt.
PlaintextAggregatedis a one-shot local event. Returning on a transientGetAggregatorSubmissionRank/getE3Stagemiss means this rank never reachespublish_plaintext_output(), even though the tx path already retries the not-ready window. Please retry/reschedule these checks instead of treatingError everyOk(false)as terminal.Also applies to: 197-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/enclave_sol_writer.rs` around lines 175 - 183, The current code treats failures from aggregator_submission_rank_for_e3 (and the related getE3Stage checks at the other block) as terminal by calling bus.err(...) and returning, which consumes the node's single PlaintextAggregated attempt; instead, change the logic in the rank/stage checks (the call sites using aggregator_submission_rank_for_e3, and the equivalent getE3Stage handling around lines 197-210) to not return on Err or on Ok(false) but to retry or reschedule the check (e.g., re-enqueue the PlaintextAggregated handling, delay and retry, or propagate a non-terminal status) so that publish_plaintext_output() can still be reached when the rank/stage becomes ready; keep the existing bus.err logging for persistent errors but ensure transient errors or not-ready responses are retried rather than causing an early return that drops the plaintext event.crates/evm/src/ciphernode_registry_sol.rs (1)
428-438:⚠️ Potential issue | 🔴 CriticalMake finalize/publish prechecks advisory instead of terminal.
CommitteeFinalizeRequestedandPublicKeyAggregatedonly schedule one local attempt. Returning ongetCommitteeStage/publicKeyHashesread failures—or on lagging non-terminal stage reads—can permanently skip the tx even thoughsend_tx_with_retry()already handles the "not ready yet" window. Only short-circuit when the read conclusively shows the committee is already finalized/published; otherwise fall through or reschedule.Also applies to: 508-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/ciphernode_registry_sol.rs` around lines 428 - 438, The current match on should_finalize_committee(...) in finalize/publish paths returns on Err or Ok(false) which makes precheck failures terminal and can permanently skip submission; change the logic in the finalize flow (the match around should_finalize_committee(...) and the analogous check using publicKeyHashes/getCommitteeStage) to only short-circuit when the read conclusively indicates the committee is already finalized/published (i.e., the definitive "already done" condition), but for read errors or non-terminal/lagging stage values do not return—either fall through to the send_tx_with_retry flow or reschedule the attempt so transient read failures or lag do not permanently skip the transaction (apply the same change to the other occurrence around lines 508-523).crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
691-719:⚠️ Potential issue | 🟠 MajorDon't stop on
GetAggregatorSubmissionRank == Nonehere.In
Sortition, this query returnsNoneboth when the node is outside the active chain and when the finalized committee is not stored yet. Clearingpending_sharesand stopping here drops valid decryptionshares during replay/startup, and this actor still has no later committee-resolution event to recover from that state. Keep it inPendingCommitteeand retry or wake it when committee resolution lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 691 - 719, The current handler for the GetAggregatorSubmissionRank response clears pending_shares and stops the actor when rank resolution yields no active rank; change it so that Ok(None) does not clear pending_shares or call ctx.stop. Specifically, in the async move map handling the result from sortition.send(GetAggregatorSubmissionRank { ... }), distinguish Ok(Some(submission_rank)) (call resolve_aggregation_duty, and if AggregationDuty::CommitteeMember call flush_pending_shares else clear & stop) from Ok(None): for Ok(None) set the actor into a pending committee state (e.g., keep or set AggregationDuty::PendingCommittee), do not clear pending_shares, and arrange a retry/wake (e.g., schedule a retry or rely on committee-resolution events) instead of ctx.stop; keep Err(err) behavior unchanged. Ensure you update references around resolve_aggregation_duty, flush_pending_shares, pending_shares, and ctx.stop accordingly.
🤖 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/threshold_plaintext_aggregator.rs`:
- Around line 254-295: The handler handle_member_expelled currently removes
shares/proofs but doesn't update the committee size, so pass the post-expulsion
active count (msg.active_count_after) into handle_member_expelled and use it to
update the aggregator's threshold/target and re-evaluate the Collecting ->
VerifyingC6 transition; specifically, add an active_count_after parameter to
handle_member_expelled and inside its state mutation use that value when in
ThresholdPlaintextAggregatorState::Collecting to recompute threshold_n/target
and trigger the same transition logic you use elsewhere (so the actor won't wait
for impossible shares), and apply the same change to the other occurrence
referenced (lines ~893-903) to keep behavior consistent.
In `@deploy/copy-secrets.sh`:
- Around line 41-45: The current branch always echoes "Created
${target}.secrets.json" even if cp or set_network_private_key fails; change the
flow so you fail fast: perform cp "$SOURCE" "${target}.secrets.json" and if that
command fails exit non‑zero (or return) immediately; then call
set_network_private_key "${target}" "${NET_KEYS[$i]}" and if it fails remove the
partially written "${target}.secrets.json" and exit non‑zero; only echo "Created
${target}.secrets.json" after both commands succeed. Ensure you reference the
same "${target}.secrets.json" filename when removing the partial file.
---
Outside diff comments:
In `@crates/sortition/src/ciphernode_selector.rs`:
- Around line 211-243: Currently the code inserts into selected_e3s before
publishing the CiphernodeSelected event, so if bus.publish(CiphernodeSelected {
... }) fails the E3 remains marked and subsequent AggregatorSelected handlers
return early; move the self.selected_e3s.insert(msg.e3_id.clone()) call to after
a successful publish (i.e., after bus.publish(...) completes without error) in
the CiphernodeSelector handling path, and apply the same fix to the other
occurrence referenced around the 258-270 block; ensure insertion only happens
when publish succeeded and that E3RequestComplete still clears selected_e3s as
before.
---
Duplicate comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 691-719: The current handler for the GetAggregatorSubmissionRank
response clears pending_shares and stops the actor when rank resolution yields
no active rank; change it so that Ok(None) does not clear pending_shares or call
ctx.stop. Specifically, in the async move map handling the result from
sortition.send(GetAggregatorSubmissionRank { ... }), distinguish
Ok(Some(submission_rank)) (call resolve_aggregation_duty, and if
AggregationDuty::CommitteeMember call flush_pending_shares else clear & stop)
from Ok(None): for Ok(None) set the actor into a pending committee state (e.g.,
keep or set AggregationDuty::PendingCommittee), do not clear pending_shares, and
arrange a retry/wake (e.g., schedule a retry or rely on committee-resolution
events) instead of ctx.stop; keep Err(err) behavior unchanged. Ensure you update
references around resolve_aggregation_duty, flush_pending_shares,
pending_shares, and ctx.stop accordingly.
In `@crates/evm/src/ciphernode_registry_sol.rs`:
- Around line 428-438: The current match on should_finalize_committee(...) in
finalize/publish paths returns on Err or Ok(false) which makes precheck failures
terminal and can permanently skip submission; change the logic in the finalize
flow (the match around should_finalize_committee(...) and the analogous check
using publicKeyHashes/getCommitteeStage) to only short-circuit when the read
conclusively indicates the committee is already finalized/published (i.e., the
definitive "already done" condition), but for read errors or
non-terminal/lagging stage values do not return—either fall through to the
send_tx_with_retry flow or reschedule the attempt so transient read failures or
lag do not permanently skip the transaction (apply the same change to the other
occurrence around lines 508-523).
In `@crates/evm/src/enclave_sol_writer.rs`:
- Around line 175-183: The current code treats failures from
aggregator_submission_rank_for_e3 (and the related getE3Stage checks at the
other block) as terminal by calling bus.err(...) and returning, which consumes
the node's single PlaintextAggregated attempt; instead, change the logic in the
rank/stage checks (the call sites using aggregator_submission_rank_for_e3, and
the equivalent getE3Stage handling around lines 197-210) to not return on Err or
on Ok(false) but to retry or reschedule the check (e.g., re-enqueue the
PlaintextAggregated handling, delay and retry, or propagate a non-terminal
status) so that publish_plaintext_output() can still be reached when the
rank/stage becomes ready; keep the existing bus.err logging for persistent
errors but ensure transient errors or not-ready responses are retried rather
than causing an early return that drops the plaintext event.
🪄 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: be377321-cb60-47d9-890c-ed8052823257
📒 Files selected for processing (16)
agent/flow-trace/00_INDEX.mdagent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/aggregator/src/committee_finalizer.rscrates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/events/src/enclave_event/aggregator_selected.rscrates/events/src/enclave_event/mod.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rscrates/sortition/Readme.mdcrates/sortition/src/ciphernode_selector.rscrates/sortition/src/sortition.rsdeploy/copy-secrets.shtests/integration/fns.sh
✅ Files skipped from review due to trivial changes (3)
- crates/events/src/enclave_event/aggregator_selected.rs
- agent/flow-trace/00_INDEX.md
- crates/sortition/Readme.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/integration/fns.sh
- agent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.md
- crates/aggregator/src/ext.rs
- agent/flow-trace/04_DKG_AND_COMPUTATION.md
- crates/aggregator/src/committee_finalizer.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/evm/src/enclave_sol_writer.rs (1)
175-183:⚠️ Potential issue | 🟠 MajorRetry the rank lookup instead of abandoning plaintext publication.
Line 179 returns on the first
GetAggregatorSubmissionRankfailure.PlaintextAggregatedis a one-shot event, so a transient sortition/mailbox miss permanently removes this node from the fallback publication chain beforepublishPlaintextOutput()'s own retry loop ever runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/enclave_sol_writer.rs` around lines 175 - 183, The code currently returns immediately on Err from aggregator_submission_rank_for_e3 in enclave_sol_writer.rs, which aborts PlaintextAggregated handling; change this to retry the rank lookup a few times (e.g., 3-5 attempts) with a short delay between tries (use tokio::time::sleep or equivalent) before giving up—on each failure log via bus.err(EType::Evm, err) or processLogger but only return after exhausting retries; keep the call site and surrounding logic in publishPlaintextOutput/PlaintextAggregated but replace the single-match with a retry loop and exponential or fixed backoff and a final error path that behaves as the original return.crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
729-743:⚠️ Potential issue | 🟠 MajorDon't stop the aggregator on the first unresolved rank lookup.
This treats both
NoneandErrfromGetAggregatorSubmissionRankas terminal, clearingpending_sharesand stopping immediately. If sortition has not hydrated this E3 yet, bufferedDecryptionshareCreatedevents are lost permanently even thoughPendingCommitteeexists for exactly that case.
🤖 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/threshold_plaintext_aggregator.rs`:
- Around line 269-285: When the actor transitions to
ThresholdPlaintextAggregatorState::VerifyingC6 after an expulsion completes (the
branch creating VerifyingC6 with fields like threshold_m, threshold_n, shares,
c6_proofs, c6_wrapped_proofs, ciphertext_output, params), it currently never
dispatches ShareVerificationDispatched and so will stall; mirror the add_share()
path by dispatching the ShareVerificationDispatched event immediately after
constructing the VerifyingC6 state (do the same fix in the other identical
location noted), ensuring the verification workflow is kicked off.
In `@crates/evm/src/ciphernode_registry_sol.rs`:
- Around line 492-499: The current code returns immediately on a failed
aggregator_submission_rank_for_e3 call which prematurely removes this node from
the publish chain; instead, retry the rank resolution in-place: when
aggregator_submission_rank_for_e3(&sortition, &e3_id, &my_address).await returns
Err(err) keep calling it (with a small async backoff or bounded retry loop)
until it succeeds or a sensible retry limit is reached, calling
bus.err(EType::Evm, err) each failure for visibility but do not return from this
block; update the logic around the local variable rank so it is only set when
Ok(rank) is returned, and preserve asynchronous flow inside the function that
calls this (e.g., publishCommittee’s context).
---
Duplicate comments:
In `@crates/evm/src/enclave_sol_writer.rs`:
- Around line 175-183: The code currently returns immediately on Err from
aggregator_submission_rank_for_e3 in enclave_sol_writer.rs, which aborts
PlaintextAggregated handling; change this to retry the rank lookup a few times
(e.g., 3-5 attempts) with a short delay between tries (use tokio::time::sleep or
equivalent) before giving up—on each failure log via bus.err(EType::Evm, err) or
processLogger but only return after exhausting retries; keep the call site and
surrounding logic in publishPlaintextOutput/PlaintextAggregated but replace the
single-match with a retry loop and exponential or fixed backoff and a final
error path that behaves as the original return.
🪄 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: 43bb248c-21b0-4b2b-9b4a-25e57833678a
📒 Files selected for processing (6)
crates/aggregator/src/committee_finalizer.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/enclave_sol_writer.rsdeploy/copy-secrets.shpackages/enclave-contracts/contracts/test/MockCiphernodeRegistry.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/copy-secrets.sh
- crates/aggregator/src/committee_finalizer.rs
Comment on lines
+269
to
+285
| if active_count_after > 0 && (data.shares.len() as u64) >= active_count_after { | ||
| info!( | ||
| e3_id = %self.e3_id, | ||
| party_id = party_id, | ||
| active_count_after = active_count_after, | ||
| "Collected all remaining active plaintext shares after expulsion, transitioning to VerifyingC6" | ||
| ); | ||
|
|
||
| ThresholdPlaintextAggregatorState::VerifyingC6(VerifyingC6 { | ||
| threshold_m: data.threshold_m, | ||
| threshold_n: active_count_after, | ||
| shares: data.shares, | ||
| c6_proofs: data.c6_proofs, | ||
| c6_wrapped_proofs: data.c6_wrapped_proofs, | ||
| ciphertext_output: data.ciphertext_output, | ||
| params: data.params, | ||
| }) |
Contributor
There was a problem hiding this comment.
Kick off C6 verification when expulsion completes collection.
This path can move the actor from Collecting to VerifyingC6, but nothing dispatches ShareVerificationDispatched afterward. Unlike the add_share() path, the actor will just sit in VerifyingC6 once active_count_after makes the reduced committee complete.
Also applies to: 926-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 269 -
285, When the actor transitions to
ThresholdPlaintextAggregatorState::VerifyingC6 after an expulsion completes (the
branch creating VerifyingC6 with fields like threshold_m, threshold_n, shares,
c6_proofs, c6_wrapped_proofs, ciphertext_output, params), it currently never
dispatches ShareVerificationDispatched and so will stall; mirror the add_share()
path by dispatching the ShareVerificationDispatched event immediately after
constructing the VerifyingC6 state (do the same fix in the other identical
location noted), ensuring the verification workflow is kicked off.
Comment on lines
+492
to
+499
| let rank = | ||
| match aggregator_submission_rank_for_e3(&sortition, &e3_id, &my_address).await { | ||
| Ok(rank) => rank, | ||
| Err(err) => { | ||
| bus.err(EType::Evm, err); | ||
| return; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Retry rank resolution instead of aborting committee publication.
At this point the node already passed the nodes.contains(&my_address) filter. Returning on the first GetAggregatorSubmissionRank error means a transient sortition/mailbox miss permanently removes this committee member from the fallback publish chain before publishCommittee()'s own retry loop runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/evm/src/ciphernode_registry_sol.rs` around lines 492 - 499, The
current code returns immediately on a failed aggregator_submission_rank_for_e3
call which prematurely removes this node from the publish chain; instead, retry
the rank resolution in-place: when aggregator_submission_rank_for_e3(&sortition,
&e3_id, &my_address).await returns Err(err) keep calling it (with a small async
backoff or bounded retry loop) until it succeeds or a sensible retry limit is
reached, calling bus.err(EType::Evm, err) each failure for visibility but do not
return from this block; update the logic around the local variable rank so it is
only set when Ok(rank) is returned, and preserve asynchronous flow inside the
function that calls this (e.g., publishCommittee’s context).
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration Changes
Documentation