feat: ciphernode fault attribution and slashing [skip-line-limit]#1323
Closed
hmzakhalid wants to merge 48 commits into
Closed
feat: ciphernode fault attribution and slashing [skip-line-limit]#1323hmzakhalid wants to merge 48 commits into
hmzakhalid wants to merge 48 commits into
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 an on‑chain SlashingManager and related ABIs, readers/writers, committee expulsion events, proof digest/encoding for on‑chain evidence, and integrates expulsion-aware flows across registry, enclave, aggregator, keyshare collectors, builder/config, deployment scripts, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant OP as Operator
participant SM as SlashingManager
participant CR as CiphernodeRegistry
participant ENC as Enclave
participant AGG as Aggregator
participant KEC as KeyshareCollector
participant TSC as ShareCollector
OP->>SM: proposeSlash(e3Id, operator, reason, proof/evidence)
SM->>SM: validate/consume evidence or record proposal
SM->>CR: expelCommitteeMember(e3Id, operator, reason)
CR->>CR: update active map, decrement activeCount
CR-->>SM: return activeCount, thresholdM
alt threshold breached
SM->>ENC: onE3Failed(e3Id, reason)
end
SM-->>OP: emit SlashExecuted / SlashProposed
par Broadcast expulsion
SM-->>AGG: CommitteeMemberExpelled
SM-->>KEC: CommitteeMemberExpelled
SM-->>TSC: CommitteeMemberExpelled
end
AGG->>AGG: remove node, flush/handle buffered keyshares
KEC->>KEC: handle ExpelPartyFromKeyCollection -> may finish collection
TSC->>TSC: handle ExpelPartyFromShareCollection -> may finish collection
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/E3RefundManager.sol (1)
134-162:⚠️ Potential issue | 🟠 MajorHonest-node allocation is permanently locked when
honestNodesis emptyIf
calculateRefundis called withhonestNodes = [](e.g.,DKGInvalidSharesfailure where every committee member misbehaved),honestNodeAmountcan be non-zero (workCompletedBps = committeeFormationBps = 1000) buthonestNodeCount = 0. There is no code path that ever moves those tokens out:
claimHonestNodeRewardreverts atrequire(dist.honestNodeCount > 0, NoRefundAvailable(e3Id))(line 289).routeSlashedFundsonly routes additional slashed funds, not the original allocation.- No admin/rescue function exists.
The tokens sit in the contract indefinitely.
🔧 Proposed fix: redirect the orphaned allocation to the requester
+ // If no honest nodes participated (all were faulty), redirect their + // allocation to the requester rather than locking the funds. + if (honestNodes.length == 0 && honestNodeAmount > 0) { + requesterAmount += honestNodeAmount; + honestNodeAmount = 0; + } + // Store distribution with the actual token used for this E3 _distributions[e3Id] = RefundDistribution({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 134 - 162, When calculateRefund is called with honestNodes.length == 0 we must avoid locking the honestNode allocation forever; modify the logic in calculateRefund to detect when honestNodes is empty and instead add honestNodeAmount into requesterAmount (set honestNodeAmount = 0) before writing _distributions[e3Id] and before emitting RefundDistributionCalculated, and ensure honestNodeCount remains 0 and no honest nodes are pushed; keep protocolAmount handling unchanged (transfer to treasury if >0). Update the stored RefundDistribution (fields requesterAmount, honestNodeAmount, honestNodeCount) so the original honest-node allocation becomes claimable by the requester via existing claim flow.
🧹 Nitpick comments (6)
packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
441-449: Missing event emission insetSlashingManagerbreaks consistency with other setters.
setEnclaveemitsEnclaveSet,setBondingRegistryemitsBondingRegistrySet, butsetSlashingManageremits nothing. Off-chain indexers and operators won't be notified of slashing manager changes.Proposed fix
Add an event and emit it:
+ event SlashingManagerSet(address indexed slashingManager); + function setSlashingManager( ISlashingManager _slashingManager ) public onlyOwner { require(address(_slashingManager) != address(0), ZeroAddress()); slashingManager = _slashingManager; + emit SlashingManagerSet(address(_slashingManager)); }🤖 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 441 - 449, The setter setSlashingManager currently updates the slashingManager state but does not emit an event, breaking parity with other setters; add a SlashingManagerSet event declaration (e.g., event SlashingManagerSet(address indexed newSlashingManager);) alongside the other events and emit SlashingManagerSet(address(_slashingManager)) at the end of setSlashingManager after assigning slashingManager to ensure off-chain indexers/operators receive the change; reference the setSlashingManager function and the slashingManager state variable when adding the event and emission.crates/events/src/ordered_set.rs (1)
36-38: Optional: add a unit test forremove.Every other public method has a corresponding test case but
removehas none. Consider adding:🧪 Proposed test
+ #[test] + fn test_remove() { + let mut set = OrderedSet::new(); + set.insert(1); + set.insert(2); + assert!(set.remove(&1)); // returns true when element present + assert!(!set.remove(&1)); // returns false when element absent + assert_eq!(set.len(), 1); + assert!(!set.contains(&1)); + assert!(set.contains(&2)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/ordered_set.rs` around lines 36 - 38, Add a unit test for OrderedSet::remove that mirrors the existing tests for insert/contains: create an OrderedSet<T> (use the same element type as other tests), insert several values, assert remove returns true for an existing value and that contains/len reflect removal, then assert remove returns false for a non-existent value; target the pub fn remove(&mut self, value: &T) -> bool in ordered_set.rs and add the test alongside the other #[cfg(test)] unit tests.crates/aggregator/src/publickey_aggregator.rs (1)
163-171:nodes.remove()return value discarded — threshold always decrements regardless of node presence.If the expelled node had not yet submitted a keyshare it will not be in
nodes, yetthreshold_nstill decrements. While this may be intentional (expected contributor is removed so the bar lowers), the code is non-idiomatic and silently handles the case wherenodes.remove()returnsfalse. Consider tying the decrement to the membership check or documenting the deliberate semantics explicitly.♻️ Suggested defensive pattern
-nodes.remove(&node.to_string()); - -if *threshold_n > 0 { - *threshold_n -= 1; - info!( - "PublicKeyAggregator: reduced threshold_n to {} after expelling {}", - threshold_n, node - ); -} +let was_member = nodes.remove(&node.to_string()); +// Always lower threshold: expelled node was an expected contributor, +// regardless of whether they had already submitted a keyshare. +if *threshold_n > 0 { + *threshold_n -= 1; + info!( + "PublicKeyAggregator: reduced threshold_n to {} after expelling {} (had submitted: {})", + threshold_n, node, was_member + ); +}🤖 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 163 - 171, The code currently calls nodes.remove(&node.to_string()) and then always decrements *threshold_n — change this to capture the boolean return (e.g., let removed = nodes.remove(&node.to_string());) and only decrement and emit the "reduced threshold_n" info when removed is true; if you intentionally want to always lower the threshold instead, instead add an explicit comment above this block explaining that semantics so it isn't mistaken for a bug. Update the code in the PublicKeyAggregator expel/remove logic (the nodes and threshold_n usage) to use the returned value from nodes.remove and adjust logging accordingly.packages/enclave-contracts/contracts/E3RefundManager.sol (3)
282-287: O(n) honest-node lookup — consider an O(1) membership mappingThe linear scan over
_honestNodes[e3Id]to verifymsg.senderis expensive for committees of non-trivial size and runs on everyclaimHonestNodeRewardcall (once per node). A companion mapping avoids this entirely.♻️ Proposed refactor
Add to storage (after
_honestNodes):+ /// `@notice` Quick membership check for honest nodes per E3 + mapping(uint256 e3Id => mapping(address node => bool isHonest)) + internal _isHonestNode;In
calculateRefund, populate it alongside the existing push loop:for (uint256 i = 0; i < honestNodes.length; i++) { _honestNodes[e3Id].push(honestNodes[i]); + _isHonestNode[e3Id][honestNodes[i]] = true; }Replace the loop in
claimHonestNodeReward:- address[] memory nodes = _honestNodes[e3Id]; - bool isHonest = false; - for (uint256 i = 0; i < nodes.length && !isHonest; i++) { - isHonest = (nodes[i] == msg.sender); - } - require(isHonest, NotHonestNode(e3Id, msg.sender)); + require(_isHonestNode[e3Id][msg.sender], NotHonestNode(e3Id, msg.sender));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 282 - 287, The current check in claimHonestNodeReward does an O(n) scan over _honestNodes[e3Id] to find msg.sender; add an O(1) membership mapping (e.g. mapping(bytes32 => mapping(address => bool)) _isHonestNode) alongside the existing _honestNodes storage, update/populate _isHonestNode inside calculateRefund when you push nodes into _honestNodes, then replace the loop in claimHonestNodeReward with a direct require(_isHonestNode[e3Id][msg.sender], NotHonestNode(e3Id, msg.sender)); also remember to clear or update the mapping where _honestNodes is cleared/modified to avoid stale entries (use the same identifiers: _honestNodes, calculateRefund, claimHonestNodeReward, NotHonestNode).
244-247: Inconsistent error style: plain strings where surrounding code uses custom errorsThe pre-upgrade guard uses
require(..., "feeToken not initialized")(string revert) while the rest of both claim functions uses typed custom errors (RefundNotCalculated,AlreadyClaimed,NotHonestNode, etc.). Consider aligning with a custom error for consistency and lower deployment/runtime gas.♻️ Proposed refactor
Define (in the interface or here):
+ error FeeTokenNotInitialized(uint256 e3Id);Then replace both occurrences:
- require( - address(dist.feeToken) != address(0), - "feeToken not initialized" - ); + if (address(dist.feeToken) == address(0)) revert FeeTokenNotInitialized(e3Id);Also applies to: 274-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 244 - 247, Define a custom error (e.g., error FeeTokenNotInitialized();) in this contract or its interface, then replace the two string-revert requires that check address(dist.feeToken) != address(0) with the custom error style: if (address(dist.feeToken) == address(0)) revert FeeTokenNotInitialized(); — update both the pre-upgrade guard and the same check used in the claim functions (where RefundNotCalculated/AlreadyClaimed/NotHonestNode are used) to use this new FeeTokenNotInitialized error for consistency and gas savings.
34-36:feeTokenandbondingRegistrystate variables are written but never readBoth are populated in
initialize(lines 91–92) but are not referenced anywhere else in the contract. All token transfers now usedist.feeToken(per-E3), and the comment at lines 305–307 explicitly acknowledges thatbondingRegistry.distributeRewardswas bypassed in favour of a direct transfer. Because this is an upgradeable contract, simply removing them would break the storage layout; they should at minimum be annotated as deprecated, or consolidated if future use is genuinely intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/E3RefundManager.sol` around lines 34 - 36, The state variables feeToken and bondingRegistry are written in initialize but never read; to avoid breaking the upgradeable storage layout, annotate both with NatSpec `@deprecated` and add a clear inline comment above feeToken and bondingRegistry stating they are intentionally unused and retained solely for storage compatibility (mention that distributions now use dist.feeToken and bondingRegistry.distributeRewards was bypassed), keep their current declarations and any assignments in initialize to preserve layout, and if you actually intend future use then instead consolidate them into a single reserved storage slot (or replace with a uint256[1] private __reserved; comment) to make the intent explicit.
🤖 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 202-209: The CommitteeMemberExpelled branch must guard against
events for other e3 instances: add an explicit check that data.e3_id ==
self.e3_id at the start of the EnclaveEventData::CommitteeMemberExpelled arm and
return early (no state mutation or call to handle_member_expelled) if it doesn't
match; mirror the pattern used in the KeyshareCreated handler (compare
data.e3_id and self.e3_id and log/return) so handle_member_expelled and any
decrement of threshold_n only run for the correct e3_id.
- Around line 211-226: handle_member_expelled is currently dispatching
ComputeAggregate whenever the state happens to be Computing, causing duplicate
dispatches; fix by snapshotting the prior state before you mutate and only call
notify_sync(...) for ComputeAggregate when the transition actually moved into
PublicKeyAggregatorState::Computing (i.e., previous state was not Computing/was
Collecting and new state is Computing). Locate handle_member_expelled and add a
prior_state = self.state.get().clone() (or equivalent) before any mutation,
perform the state mutation, then check that prior_state !=
PublicKeyAggregatorState::Computing && matches!(self.state.get(),
Some(PublicKeyAggregatorState::Computing{..})) before invoking
notify_sync(TypedEvent::new(ComputeAggregate{...}, ...)); this prevents
duplicate ComputeAggregate dispatches that lead
Handler<TypedEvent<ComputeAggregate>> (set_pubkey / PublicKeyAggregated) from
firing twice.
In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol`:
- Around line 496-506: The call to enclave.onE3Failed inside _executeSlash can
revert (e.g., E3AlreadyFailed) and abort processing of other penalties; wrap the
enclave.onE3Failed(p.e3Id, p.failureReason) invocation in a try/catch so that
known benign reverts (such as E3AlreadyFailed) are swallowed and processing
continues, while unexpected errors are rethrown; keep the surrounding logic that
checks p.affectsCommittee and uses ciphernodeRegistry.expelCommitteeMember
(which returns activeCount and thresholdM) and only perform the try/catch when
activeCount < thresholdM and p.failureReason > 0 so other slash effects
(slashTicketBalance, slashLicenseBond, bans) always execute even if the E3 has
already been marked failed.
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Around line 883-902: The test currently asserts that slashing via
slashingManager.proposeSlash reverts with E3AlreadyFailed because onE3Failed
bubbles up; after wrapping onE3Failed calls in a try-catch inside
SlashingManager._executeSlash, update this test to expect the proposeSlash call
to succeed and to assert that the operator's financial penalties were applied
while the enclave's E3 state remains Failed; specifically modify expectations
around proposeSlash (call to proposeSlash on operator3), remove the
revertedWithCustomError(enclave, "E3AlreadyFailed") check, and add assertions
that balances/penalties (the same checks used elsewhere in this spec) reflect
the third slash while verifying E3 is still Failed.
---
Outside diff comments:
In `@packages/enclave-contracts/contracts/E3RefundManager.sol`:
- Around line 134-162: When calculateRefund is called with honestNodes.length ==
0 we must avoid locking the honestNode allocation forever; modify the logic in
calculateRefund to detect when honestNodes is empty and instead add
honestNodeAmount into requesterAmount (set honestNodeAmount = 0) before writing
_distributions[e3Id] and before emitting RefundDistributionCalculated, and
ensure honestNodeCount remains 0 and no honest nodes are pushed; keep
protocolAmount handling unchanged (transfer to treasury if >0). Update the
stored RefundDistribution (fields requesterAmount, honestNodeAmount,
honestNodeCount) so the original honest-node allocation becomes claimable by the
requester via existing claim flow.
---
Duplicate comments:
In `@crates/evm/src/slashing_manager_sol_reader.rs`:
- Around line 29-36: The code currently uses panicking .to::<u128>() conversions
for U256 fields when constructing e3_events::SlashExecuted inside
EnclaveEventData::from; replace those with fallible conversions (e.g.,
try_into::<u128>()) and handle failures instead of panicking — for example,
attempt conversion for proposal_id, ticket_amount, and license_amount, and if
any try_into fails, return None (or log and skip the event) so
malformed/malicious U256 values don’t crash the node. Ensure you update the
construction of the e3_events::SlashExecuted instance (referenced in
EnclaveEventData::from) to use the try_into results (mapping to u128 on Ok or
short-circuiting to None/Error on Err).
---
Nitpick comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 163-171: The code currently calls nodes.remove(&node.to_string())
and then always decrements *threshold_n — change this to capture the boolean
return (e.g., let removed = nodes.remove(&node.to_string());) and only decrement
and emit the "reduced threshold_n" info when removed is true; if you
intentionally want to always lower the threshold instead, instead add an
explicit comment above this block explaining that semantics so it isn't mistaken
for a bug. Update the code in the PublicKeyAggregator expel/remove logic (the
nodes and threshold_n usage) to use the returned value from nodes.remove and
adjust logging accordingly.
In `@crates/events/src/ordered_set.rs`:
- Around line 36-38: Add a unit test for OrderedSet::remove that mirrors the
existing tests for insert/contains: create an OrderedSet<T> (use the same
element type as other tests), insert several values, assert remove returns true
for an existing value and that contains/len reflect removal, then assert remove
returns false for a non-existent value; target the pub fn remove(&mut self,
value: &T) -> bool in ordered_set.rs and add the test alongside the other
#[cfg(test)] unit tests.
In `@packages/enclave-contracts/contracts/E3RefundManager.sol`:
- Around line 282-287: The current check in claimHonestNodeReward does an O(n)
scan over _honestNodes[e3Id] to find msg.sender; add an O(1) membership mapping
(e.g. mapping(bytes32 => mapping(address => bool)) _isHonestNode) alongside the
existing _honestNodes storage, update/populate _isHonestNode inside
calculateRefund when you push nodes into _honestNodes, then replace the loop in
claimHonestNodeReward with a direct require(_isHonestNode[e3Id][msg.sender],
NotHonestNode(e3Id, msg.sender)); also remember to clear or update the mapping
where _honestNodes is cleared/modified to avoid stale entries (use the same
identifiers: _honestNodes, calculateRefund, claimHonestNodeReward,
NotHonestNode).
- Around line 244-247: Define a custom error (e.g., error
FeeTokenNotInitialized();) in this contract or its interface, then replace the
two string-revert requires that check address(dist.feeToken) != address(0) with
the custom error style: if (address(dist.feeToken) == address(0)) revert
FeeTokenNotInitialized(); — update both the pre-upgrade guard and the same check
used in the claim functions (where
RefundNotCalculated/AlreadyClaimed/NotHonestNode are used) to use this new
FeeTokenNotInitialized error for consistency and gas savings.
- Around line 34-36: The state variables feeToken and bondingRegistry are
written in initialize but never read; to avoid breaking the upgradeable storage
layout, annotate both with NatSpec `@deprecated` and add a clear inline comment
above feeToken and bondingRegistry stating they are intentionally unused and
retained solely for storage compatibility (mention that distributions now use
dist.feeToken and bondingRegistry.distributeRewards was bypassed), keep their
current declarations and any assignments in initialize to preserve layout, and
if you actually intend future use then instead consolidate them into a single
reserved storage slot (or replace with a uint256[1] private __reserved; comment)
to make the intent explicit.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 441-449: The setter setSlashingManager currently updates the
slashingManager state but does not emit an event, breaking parity with other
setters; add a SlashingManagerSet event declaration (e.g., event
SlashingManagerSet(address indexed newSlashingManager);) alongside the other
events and emit SlashingManagerSet(address(_slashingManager)) at the end of
setSlashingManager after assigning slashingManager to ensure off-chain
indexers/operators receive the change; reference the setSlashingManager function
and the slashingManager state variable when adding the event and emission.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/enclave-contracts/contracts/slashing/SlashingManager.sol (3)
190-204:setCiphernodeRegistryandsetEnclaveemit no events.Critical address changes on these two external dependencies are unobservable off-chain.
_hashTypedDataV4in OpenZeppelin is paired withECDSA.recoverfor signer verification — similarly, admin setters that alter verifier/registry addresses should be observable via events so monitoring infrastructure can react.Consider emitting events (e.g.,
CiphernodeRegistryUpdated,EnclaveUpdated) — consistent with thesetBondingRegistrypattern, which also lacks events. Both are minor observability gaps.♻️ Proposed change
+ event CiphernodeRegistryUpdated(address indexed newRegistry); + event EnclaveUpdated(address indexed newEnclave); function setCiphernodeRegistry( address newCiphernodeRegistry ) external onlyRole(DEFAULT_ADMIN_ROLE) { require(newCiphernodeRegistry != address(0), ZeroAddress()); ciphernodeRegistry = ICiphernodeRegistry(newCiphernodeRegistry); + emit CiphernodeRegistryUpdated(newCiphernodeRegistry); } function setEnclave( address newEnclave ) external onlyRole(DEFAULT_ADMIN_ROLE) { require(newEnclave != address(0), ZeroAddress()); enclave = IEnclave(newEnclave); + emit EnclaveUpdated(newEnclave); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol` around lines 190 - 204, Add observable events for admin address changes: declare events like CiphernodeRegistryUpdated(address indexed previous, address indexed current) and EnclaveUpdated(address indexed previous, address indexed current) (and mirror for BondingRegistry if desired), then update setCiphernodeRegistry and setEnclave to emit the corresponding event after the require and assignment, passing the previous address and newEnclave/newCiphernodeRegistry; follow the same pattern used for setBondingRegistry (include indexed addresses for off-chain filtering) so off-chain monitoring can detect these updates.
502-504: Emptycatch {}silently discards allonE3Failederrors.The try-catch correctly prevents financial penalties from being blocked (addressing the prior concern), but
catch {}discards unexpected errors too — including call failures or logic errors in theIEnclaveimplementation — with no diagnostic trace. Emitting a lightweight event on catch failure improves observability at negligible gas cost.♻️ Proposed change
+ event E3FailureSignalSuppressed(uint256 indexed e3Id, bytes32 failureReason); if (activeCount < thresholdM && p.failureReason > 0) { - try enclave.onE3Failed(p.e3Id, p.failureReason) {} catch {} + try enclave.onE3Failed(p.e3Id, p.failureReason) {} catch { + emit E3FailureSignalSuppressed(p.e3Id, p.failureReason); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol` around lines 502 - 504, The empty catch on the enclave.onE3Failed call in SlashingManager silently drops failures; update the try/catch around enclave.onE3Failed(p.e3Id, p.failureReason) inside the failure-handling block so the catch emits a lightweight event (e.g., OnE3FailedCallbackError) recording p.e3Id, p.failureReason and the address of the enclave or the caller, instead of swallowing errors; keep the try/catch to avoid reverting but log the failure information for observability.
383-393:_verifyCommitteeMembershipperforms an O(n) scan on every Lane A proposal.For committees with many members this adds proportional gas cost to the already heavy
proposeSlashpath (which also calls the ZK verifier). Consider requesting aisCommitteeMember(uint256 e3Id, address operator) external view returns (bool)view onICiphernodeRegistryto replace the full array fetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol` around lines 383 - 393, The current O(n) scan in SlashingManager.sol (in the _verifyCommitteeMembership/proposeSlash path) fetches committeeNodes via ciphernodeRegistry.getCommitteeNodes(e3Id) and loops to check membership; replace this with a new ICiphernodeRegistry view function isCommitteeMember(uint256 e3Id, address operator) external view returns (bool), then call ciphernodeRegistry.isCommitteeMember(e3Id, operator) and require it (require(..., OperatorNotInCommittee())); update the ICiphernodeRegistry interface and all registry implementations to provide an efficient membership check (e.g., mapping lookup) instead of returning the full array.packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts (3)
534-534: ExtractE3Stage.Failed = 6andfailureReasonvalues into named constants.
6appears in seven places as theE3Stage.Failedsentinel, and4/11appear asfailureReasonenum values in the slash policy setup. If the enum ordering ever shifts, all these assertions will silently misfire. Centralising them makes the intent explicit and the tests resilient.♻️ Proposed refactor
+ // E3Stage enum values (must match IEnclave.sol) + const E3Stage = { Failed: 6 } as const; + + // FailureReason enum values (must match IEnclave.sol) + const FailureReason = { + DKGInvalidShares: 4, + DecryptionInvalidShares: 11, + } as const;Then replace every raw
6/4/11reference:- expect(stage).to.equal(6); // E3Stage.Failed + expect(stage).to.equal(E3Stage.Failed); - expect(reason).to.equal(4); + expect(reason).to.equal(FailureReason.DKGInvalidShares);And in the slash policy setup:
- failureReason: 4, // FailureReason.DKGInvalidShares + failureReason: FailureReason.DKGInvalidShares, - failureReason: 11, // FailureReason.DecryptionInvalidShares + failureReason: FailureReason.DecryptionInvalidShares,Also applies to: 580-602, 779-779, 823-824, 880-906
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts` at line 534, Introduce named constants for the magic numeric enums and use them everywhere: define constants like FAILED_STAGE = E3Stage.Failed (or 6) and named failure reasons e.g. FAILURE_REASON_X = 4 and FAILURE_REASON_Y = 11 at the top of CommitteeExpulsion.spec.ts, then replace all raw occurrences of 6, 4, and 11 in assertions (e.g. the expect(stage).to.not.equal(6) checks) and in the slash policy setup blocks with these constants (reference the E3Stage symbol and the failureReason fields in the slash policy configuration to locate replacements); ensure all seven places mentioned (and the ranges 580-602, 779, 823-824, 880-906) use the new constants so tests remain correct if enum ordering changes.
95-100:e3IdandchainIdtyped asnumberinstead ofbigint.
uint256contract values are returned asBigIntin ethers.js v6. Typing these parameters asnumber(53-bit precision) is inconsistent with the rest of the ABI layer — while harmless for test IDs, it narrows the function's safe range and invites subtle overflow bugs if IDs grow.♻️ Proposed fix
- e3Id: number, + e3Id: bigint | number, verifierAddress: string, zkProof: string = "0x1234", publicInputs: string[] = [ethers.ZeroHash], - chainId: number = 31337, + chainId: bigint | number = 31337, proofType: number = 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts` around lines 95 - 100, The parameters e3Id and chainId are declared as number but should be bigint to match ethers v6 uint256 handling; update the function signature in CommitteeExpulsion.spec.ts to type e3Id and chainId as bigint, change their default values to bigint literals (e.g., 31337n) or construct with BigInt(...) where used, and audit any callers/operations in the file (comparisons, arithmetic, fixtures) to use bigint-safe operations so the test uses BigInt-compatible IDs throughout.
433-491: Consider adding token balance assertions to complement the event-based checks.The "committee expulsion via proposeSlash" tests verify committee state and event emissions but don't check that the financial penalties (
ticketPenalty: 10 USDC,licensePenalty: 50 ENCL) are actually deducted from the operator's bond. ThebondingRegistryandusdcToken/enclTokenare returned fromsetupbut go unused in these tests. An on-chain state assertion (e.g. checking the operator's ticket balance before/after) would provide stronger confidence that the slash execution path, not just the event path, is exercised end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts` around lines 433 - 491, Test doesn't assert token/bond changes after proposeSlash; add before/after balance checks to ensure penalties applied. Capture operator1 address and read balances from bondingRegistry (or its getter such as getBond/getOperatorBond) and the token contracts usdcToken.balanceOf(op1Address) and enclToken.balanceOf(op1Address) (or the specific ticket/license balance accessors if present) before calling slashingManager.proposeSlash, then assert the USDC ticket penalty (10) and ENCL license penalty (50) have been deducted from those balances after the tx; use the same fixtures (slashingManager, bondingRegistry, usdcToken, enclToken, operator1) and keep existing event and registry assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol`:
- Around line 451-456: The verifier staticcall may succeed but return fewer than
32 bytes, causing abi.decode(returnData, (bool)) to panic; update the post-call
checks in the block that calls policyVerifier.staticcall (which encodes
ICircuitVerifier.verify with zkProof and publicInputs) to guard returnData
length before decoding—after require(callSuccess, VerifierCallFailed()) add a
require(returnData.length >= 32, VerifierCallFailed()) (or == 32 if strict) and
only then abi.decode to produce proofValid, preserving the existing revert to
ProofIsValid() when proofValid is true.
---
Duplicate comments:
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Around line 883-906: The test was updated to assert that slashing an
already-Failed E3 still succeeds and emits the SlashExecuted event, because
_executeSlash wraps the onE3Failed call in try-catch so penalties are applied
idempotently; ensure the proposeSlash call in the test (calling
slashingManager.proposeSlash) continues to expect emit SlashExecuted, that the
proof is generated with signAndEncodeProof for operator3, and that the final
assertion checks enclave.getE3Stage(0) equals the Failed state (6); no code
change to contract logic is required—keep references to proposeSlash,
_executeSlash, onE3Failed, signAndEncodeProof, getE3Stage, and the SlashExecuted
event consistent.
---
Nitpick comments:
In `@packages/enclave-contracts/contracts/slashing/SlashingManager.sol`:
- Around line 190-204: Add observable events for admin address changes: declare
events like CiphernodeRegistryUpdated(address indexed previous, address indexed
current) and EnclaveUpdated(address indexed previous, address indexed current)
(and mirror for BondingRegistry if desired), then update setCiphernodeRegistry
and setEnclave to emit the corresponding event after the require and assignment,
passing the previous address and newEnclave/newCiphernodeRegistry; follow the
same pattern used for setBondingRegistry (include indexed addresses for
off-chain filtering) so off-chain monitoring can detect these updates.
- Around line 502-504: The empty catch on the enclave.onE3Failed call in
SlashingManager silently drops failures; update the try/catch around
enclave.onE3Failed(p.e3Id, p.failureReason) inside the failure-handling block so
the catch emits a lightweight event (e.g., OnE3FailedCallbackError) recording
p.e3Id, p.failureReason and the address of the enclave or the caller, instead of
swallowing errors; keep the try/catch to avoid reverting but log the failure
information for observability.
- Around line 383-393: The current O(n) scan in SlashingManager.sol (in the
_verifyCommitteeMembership/proposeSlash path) fetches committeeNodes via
ciphernodeRegistry.getCommitteeNodes(e3Id) and loops to check membership;
replace this with a new ICiphernodeRegistry view function
isCommitteeMember(uint256 e3Id, address operator) external view returns (bool),
then call ciphernodeRegistry.isCommitteeMember(e3Id, operator) and require it
(require(..., OperatorNotInCommittee())); update the ICiphernodeRegistry
interface and all registry implementations to provide an efficient membership
check (e.g., mapping lookup) instead of returning the full array.
In `@packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Line 534: Introduce named constants for the magic numeric enums and use them
everywhere: define constants like FAILED_STAGE = E3Stage.Failed (or 6) and named
failure reasons e.g. FAILURE_REASON_X = 4 and FAILURE_REASON_Y = 11 at the top
of CommitteeExpulsion.spec.ts, then replace all raw occurrences of 6, 4, and 11
in assertions (e.g. the expect(stage).to.not.equal(6) checks) and in the slash
policy setup blocks with these constants (reference the E3Stage symbol and the
failureReason fields in the slash policy configuration to locate replacements);
ensure all seven places mentioned (and the ranges 580-602, 779, 823-824,
880-906) use the new constants so tests remain correct if enum ordering changes.
- Around line 95-100: The parameters e3Id and chainId are declared as number but
should be bigint to match ethers v6 uint256 handling; update the function
signature in CommitteeExpulsion.spec.ts to type e3Id and chainId as bigint,
change their default values to bigint literals (e.g., 31337n) or construct with
BigInt(...) where used, and audit any callers/operations in the file
(comparisons, arithmetic, fixtures) to use bigint-safe operations so the test
uses BigInt-compatible IDs throughout.
- Around line 433-491: Test doesn't assert token/bond changes after
proposeSlash; add before/after balance checks to ensure penalties applied.
Capture operator1 address and read balances from bondingRegistry (or its getter
such as getBond/getOperatorBond) and the token contracts
usdcToken.balanceOf(op1Address) and enclToken.balanceOf(op1Address) (or the
specific ticket/license balance accessors if present) before calling
slashingManager.proposeSlash, then assert the USDC ticket penalty (10) and ENCL
license penalty (50) have been deducted from those balances after the tx; use
the same fixtures (slashingManager, bondingRegistry, usdcToken, enclToken,
operator1) and keep existing event and registry assertions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/keyshare/src/threshold_keyshare.rs (1)
389-417: Expulsion forwarding logic is clean and consistent.The party_id filter correctly complements the
PublicKeyAggregator's inverse filter (raw-only vs enriched-only), anddo_sendis appropriate for fire-and-forget forwarding to the collectors.One minor observation: if either collector is
None(e.g., becauseCiphernodeSelectedprocessing failed earlier), the expulsion is silently dropped with no log. Consider adding awarn!if a collector is unexpectedlyNoneto aid debugging, though this is unlikely during normal operation.💡 Optional: add a warning when collectors are missing
if let Some(ref collector) = self.encryption_key_collector { collector.do_send(ExpelPartyFromKeyCollection { party_id, ec: ec.clone(), }); + } else { + warn!("encryption_key_collector not available for expulsion of party {}", party_id); } if let Some(ref collector) = self.decryption_key_collector { collector.do_send(ExpelPartyFromShareCollection { party_id, ec }); + } else { + warn!("decryption_key_collector not available for expulsion of party {}", party_id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 389 - 417, The expulsion handler handle_committee_member_expelled currently drops events silently when either encryption_key_collector or decryption_key_collector is None; update the function to log a warning when a collector is missing so unexpected None cases are visible in logs—check encryption_key_collector and decryption_key_collector before calling do_send and emit warn! with context (party_id, node/address, e3_id) for the missing collector(s), while preserving the existing do_send calls to ExpelPartyFromKeyCollection and ExpelPartyFromShareCollection and the early return behavior for None party_id.
🤖 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/keyshare_created_filter_buffer.rs`:
- Around line 73-90: The removal of expelled nodes in
KeyshareCreatedFilterBuffer fails because
EnclaveEventData::CommitteeMemberExpelled builds node_addr with format!("{:?}",
data.node) (Debug) which doesn't match the HashSet<String> entries created via
Address::to_string(); change the creation of node_addr to use
data.node.to_string() (Display) so committee.remove(&node_addr) actually finds
and removes the entry, keeping the committee HashSet consistent and preventing
late KeyshareCreated events from expelled nodes.
In `@crates/events/src/committee.rs`:
- Around line 15-22: The derived PartialEq currently compares the skipped index
field causing deserialized Committee instances (index empty due to
#[serde(skip)]) to differ from those created by Committee::new(); fix by either
implementing custom Deserialize for Committee that rebuilds the index from
members during deserialization, or implement PartialEq manually for Committee so
equality only compares the members Vec<String> (i.e., compare self.members ==
other.members) and ignores index; reference the Committee struct, its members
and index fields, and Committee::new()/Deserialize paths when making the change.
- Around line 35-47: The lazy-rebuild never mutates self because party_id_for
takes &self and index is a plain HashMap; change the struct's index field to an
interior-mutable OnceCell<HashMap<String,u64>> (or OnceCell/RefCell variant you
prefer), update the field type wherever referenced, and modify party_id_for to
call index.get_or_init(|| build_map_from(&self.members)) and then read from the
populated map; this preserves lazy rebuild after deserialization without needing
a custom Deserialize impl.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 38-50: Remove the three unused imports from the top import block:
delete Mutex from the sync import, and remove ChaCha20Rng and SeedableRng from
the rand-related imports so only the used items (e.g., OsRng, HashMap, mem)
remain; update the use statements that currently include Mutex, ChaCha20Rng, and
SeedableRng to no longer reference those symbols to eliminate the unused-import
warnings.
---
Nitpick comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 389-417: The expulsion handler handle_committee_member_expelled
currently drops events silently when either encryption_key_collector or
decryption_key_collector is None; update the function to log a warning when a
collector is missing so unexpected None cases are visible in logs—check
encryption_key_collector and decryption_key_collector before calling do_send and
emit warn! with context (party_id, node/address, e3_id) for the missing
collector(s), while preserving the existing do_send calls to
ExpelPartyFromKeyCollection and ExpelPartyFromShareCollection and the early
return behavior for None party_id.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/keyshare/src/threshold_keyshare.rs (1)
389-417: Silent drop when both collectors areNone— add a diagnostic warning.When
party_idisSome(i.e., the event is fully enriched) but bothencryption_key_collectoranddecryption_key_collectorareNone(the collectors haven't been initialised yet or were already cleared), the expulsion is forwarded to neither and the method returns silently. This makes it difficult to distinguish an expected no-op (post-E3RequestComplete) from an unexpected early-arrival case during debugging.A
warn!log guarded on at least one collector being absent would make the behaviour observable:+ if self.encryption_key_collector.is_none() && self.decryption_key_collector.is_none() { + warn!( + "CommitteeMemberExpelled received but no active collectors for e3_id={}; ignoring", + data.e3_id + ); + return; + } + if let Some(ref collector) = self.encryption_key_collector {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 389 - 417, In handle_committee_member_expelled, add a diagnostic warning when the enriched event (party_id present) will be dropped because both encryption_key_collector and decryption_key_collector are None; check the Option values for encryption_key_collector and decryption_key_collector and if both are None (or if either is None, optionally warn which one is missing), emit a warn! including node (data.node), party_id and e3_id so the absence is observable; otherwise keep the existing do_send calls to ExpelPartyFromKeyCollection and ExpelPartyFromShareCollection unchanged.crates/aggregator/src/keyshare_created_filter_buffer.rs (1)
68-71: Themsg.clone()on line 69 is unnecessary and can be removed.Since
get_data()returns a reference (&EnclaveEventData), thedatavariable bound in the pattern match is a reference that doesn't own or requiremsgto remain alive. Thedatais used only to extractcommittee(which is cloned separately), somsgcan be moved intodo_send()without cloning. This matches the pattern in other match arms wheremsgis moved directly without cloning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/keyshare_created_filter_buffer.rs` around lines 68 - 71, The match arm for EnclaveEventData::CommitteeFinalized unnecessarily clones msg before sending; change the send to move msg into self.dest.do_send(...) (remove msg.clone()) and keep extracting committee via data.committee.iter().cloned().collect() and calling self.process_buffered_events() as before so ownership is correct and consistent with other arms that move msg.
🤖 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/request/src/router.rs`:
- Around line 205-218: Remove the duplicate completion trigger by deleting the
match arm handling EnclaveEventData::E3Failed so only the
EnclaveEventData::E3StageChanged(ref data) arm (matching E3Stage::Complete |
E3Stage::Failed) publishes E3RequestComplete; specifically, remove the block
that constructs E3RequestComplete for EnclaveEventData::E3Failed and calls
self.bus.publish(...), leaving the E3StageChanged branch to be the single place
that creates the E3RequestComplete event and calls self.bus.publish(event,
ctx)?.
In `@crates/sortition/src/sortition.rs`:
- Around line 361-365: decrement_jobs_for_e3 currently removes the entry from
finalized_committees immediately, which can race with CommitteeMemberExpelled
enrichment that calls get_committee(&data.e3_id) and loses party_id if the
terminal event is processed first; change the logic in decrement_jobs_for_e3
(and the finalized_committees management) to retain the committee until all
expected expulsions are handled — e.g. add a per-E3 expulsion_pending counter or
flag in the committee struct, increment when an expulsion is first observed (or
initialize from expected count), decrement when CommitteeMemberExpelled
enrichment completes, and only remove the entry from finalized_committees when
the pending counter reaches zero (or after an explicit completion path like
E3StageChanged/PlaintextOutputPublished that confirms no pending expulsions).
Ensure CommitteeMemberExpelled handler consults and updates this counter/flag
rather than relying on existence check alone so party_id enrichment is
preserved.
---
Duplicate comments:
In `@crates/aggregator/src/keyshare_created_filter_buffer.rs`:
- Around line 73-90: The expelled-node string is built with Debug formatting
causing mismatch with the committee HashSet entries (which use
Display/to_string); in the EnclaveEventData::CommitteeMemberExpelled handling,
replace the Debug-based node_addr creation (format!("{:?}", data.node)) with
Display/to_string (e.g., data.node.to_string() or format!("{}", data.node)) so
that committee.remove(&node_addr) correctly matches and removes the expelled
member; update the code in the KeyshareCreatedFilterBuffer where
committee.remove(&node_addr) is called to use the new node_addr.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 38-44: Remove the unused imports SeedableRng, ChaCha20Rng, and
Mutex from the use statements at the top of threshold_keyshare.rs; leave only
the actually used symbols (OsRng, Arc, HashMap, mem) so the module imports match
usage and eliminate the unused-import warnings referring to SeedableRng,
ChaCha20Rng, and Mutex.
---
Nitpick comments:
In `@crates/aggregator/src/keyshare_created_filter_buffer.rs`:
- Around line 68-71: The match arm for EnclaveEventData::CommitteeFinalized
unnecessarily clones msg before sending; change the send to move msg into
self.dest.do_send(...) (remove msg.clone()) and keep extracting committee via
data.committee.iter().cloned().collect() and calling
self.process_buffered_events() as before so ownership is correct and consistent
with other arms that move msg.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 389-417: In handle_committee_member_expelled, add a diagnostic
warning when the enriched event (party_id present) will be dropped because both
encryption_key_collector and decryption_key_collector are None; check the Option
values for encryption_key_collector and decryption_key_collector and if both are
None (or if either is None, optionally warn which one is missing), emit a warn!
including node (data.node), party_id and e3_id so the absence is observable;
otherwise keep the existing do_send calls to ExpelPartyFromKeyCollection and
ExpelPartyFromShareCollection unchanged.
Collaborator
Author
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
Tests