fix: fail pk and plaintext ag err path [skip-line-limit]#1537
Open
hmzakhalid wants to merge 1 commit intomainfrom
Open
fix: fail pk and plaintext ag err path [skip-line-limit]#1537hmzakhalid wants to merge 1 commit intomainfrom
hmzakhalid wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR implements explicit failure signaling for public key and plaintext aggregation paths. It updates flow trace documentation, adds correlation-tracked error handling with E3Failed emission to PublicKeyAggregator and ThresholdPlaintextAggregator, introduces a decryption-round failure helper, and expands test coverage for error scenarios. ChangesAggregation Explicit Failure Paths
Sequence DiagramsequenceDiagram
participant Agg as PublicKeyAggregator/<br/>ThresholdPlaintextAggregator
participant Worker as ComputeWorker
participant Event as E3 Event System
Agg->>Agg: Create ComputeRequest<br/>with CorrelationId
Agg->>Worker: Dispatch DkgAggregation<br/>or CalculateDecryption
Note over Worker: Compute step fails
Worker-->>Agg: ComputeRequestError
rect rgba(255, 100, 100, 0.5)
Agg->>Agg: handle_compute_request_error:<br/>Match correlation, verify async context
Agg->>Agg: Clear in-flight state<br/>(correlation, proofs, pending flags)
Agg->>Event: Emit E3Failed<br/>with failure reason
end
Event-->>Agg: Event published
Agg->>Agg: Round halted explicitly<br/>with clear failure signal
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
hmzakhalid
changed the title
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 47-52: The doc claims computation-stage failures from
ThresholdKeyshare (GenPkShareAndSkSss, GenEsiSss, CalculateDecryptionKey,
CalculateDecryptionShare) emit E3Failed, but the code currently propagates
errors via the ? operator into trap() producing EnclaveError and
ThresholdKeyshare also doesn't subscribe to ComputeRequestError; fix by wiring
these computation-response handlers to explicitly emit E3Failed events instead
of using ? into trap() and ensure ThresholdKeyshare subscribes to
ComputeRequestError; update the four handler implementations (functions handling
GenPkShareAndSkSss, GenEsiSss, CalculateDecryptionKey, CalculateDecryptionShare)
to convert errors into the E3Failed emission path, and add subscription for
ComputeRequestError in the ThresholdKeyshare setup so computation failures
trigger the E3Failed path rather than EnclaveError.
🪄 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: 0f300a69-6c5f-4e64-960b-0ce6f2acb056
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
agent/flow-trace/00_INDEX.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/aggregator/Cargo.tomlcrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
Comment on lines
+47
to
+52
| │ └─ Collector timeouts are derived from the DKG stage budget: | ||
| │ ├─ shared base window from `E3_DKG_WINDOW_SECS` (default 7200s, | ||
| │ │ matching current production `Enclave` deployment config) | ||
| │ ├─ EncryptionKeyCollector cutoff at 10% of the DKG window | ||
| │ ├─ ThresholdShareCollector cutoff at 60% of the DKG window | ||
| │ └─ per-collector env vars still override these derived defaults |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ThresholdKeyshare actually emits E3Failed for the stages described in 04_DKG_AND_COMPUTATION.md.
fd -t f 'threshold_keyshare.rs|threshold_keyshare/' -X rg -nP -C3 'E3Failed|EncryptionKeyCollectionFailed|ThresholdShareCollectionFailed|CalculateDecryptionShare|CalculateDecryptionKey|GenEsiSss|GenPkShareAndSkSss' {} \;
rg -nP --type=rust -C2 'E3Failed\s*\{' -g '!crates/aggregator/**'Repository: gnosisguild/enclave
Length of output: 19851
🏁 Script executed:
# Look at the full implementation of computation response handlers in threshold_keyshare.rs
fd -type f -name 'threshold_keyshare.rs' -x sh -c '
echo "=== handle_gen_pk_share_and_sk_sss_response ==="
sed -n "914,960p" "$1"
echo ""
echo "=== handle_gen_esi_sss_response ==="
sed -n "997,1010p" "$1"
echo ""
echo "=== handle_calculate_decryption_key_response ==="
sed -n "1837,1900p" "$1"
echo ""
echo "=== handle_calculate_decryption_share_response ==="
sed -n "2136,2180p" "$1"
' sh {} \;Repository: gnosisguild/enclave
Length of output: 234
🏁 Script executed:
# Find and read the threshold_keyshare.rs file
fd 'threshold_keyshare.rs' | head -1 | xargs -I {} sh -c '
echo "=== handle_gen_pk_share_and_sk_sss_response ==="
sed -n "914,960p" "$1"
echo ""
echo "=== handle_gen_esi_sss_response ==="
sed -n "997,1010p" "$1"
echo ""
echo "=== handle_calculate_decryption_key_response ==="
sed -n "1837,1900p" "$1"
echo ""
echo "=== handle_calculate_decryption_share_response ==="
sed -n "2136,2180p" "$1"
' sh {}Repository: gnosisguild/enclave
Length of output: 7158
🏁 Script executed:
# Check where handle_compute_response is called and how errors are handled
rg -n 'handle_compute_response|pub fn handle_compute_response' crates/keyshare/src/threshold_keyshare.rs -B 2 -A 10 | head -100Repository: gnosisguild/enclave
Length of output: 1032
🏁 Script executed:
# Find the trap function definition
rg -n 'fn trap|pub fn trap' -A 15 --type rust | head -60Repository: gnosisguild/enclave
Length of output: 2231
🏁 Script executed:
# Find the ErrorDispatcher trait and its err method implementation
rg -n 'trait ErrorDispatcher|fn err|pub fn err' -A 10 crates/events/src/enclave_event/enclave_error.rs | head -80Repository: gnosisguild/enclave
Length of output: 904
🏁 Script executed:
# Find BusHandle and its err method
rg -n 'impl.*ErrorDispatcher.*BusHandle|fn err' crates/events/src/ -A 10 | grep -A 15 'BusHandle'Repository: gnosisguild/enclave
Length of output: 3737
🏁 Script executed:
# Find the event_from_error method
rg -n 'fn event_from_error|event_from_error' crates/events/src/bus_handle.rs -A 20 | head -80Repository: gnosisguild/enclave
Length of output: 1934
🏁 Script executed:
# Find the EnclaveEvent::from_error implementation
rg -n 'fn from_error' crates/events/src/ -A 20 | head -100Repository: gnosisguild/enclave
Length of output: 6451
🏁 Script executed:
# Check if there are any error handlers that convert EnclaveError to E3Failed
rg -n 'EnclaveError|EnclaveEventData::EnclaveError' crates/keyshare/src/threshold_keyshare.rs -B 3 -A 5Repository: gnosisguild/enclave
Length of output: 45
🏁 Script executed:
# Check how EnclaveError events are handled in threshold_keyshare.rs
rg -n 'EnclaveEventData::' crates/keyshare/src/threshold_keyshare.rs | head -20Repository: gnosisguild/enclave
Length of output: 1082
🏁 Script executed:
# Check if EnclaveError events are handled anywhere that might convert them to E3Failed
rg -n 'EnclaveError|Handler.*EnclaveError' crates/keyshare/src/ -B 2 -A 10Repository: gnosisguild/enclave
Length of output: 45
🏁 Script executed:
# Search for any handlers or converters that might turn EnclaveError into E3Failed
rg -n 'EnclaveEventData::EnclaveError' crates/ -B 3 -A 5Repository: gnosisguild/enclave
Length of output: 1568
🏁 Script executed:
# Read the specific lines mentioned in the review comment
sed -n '47,52p;114,121p;174,182p;188,194p;323,330p;688,694p' agent/flow-trace/04_DKG_AND_COMPUTATION.mdRepository: gnosisguild/enclave
Length of output: 2318
🏁 Script executed:
# Search for ComputeRequestError handling
rg -n 'ComputeRequestError' crates/keyshare/src/threshold_keyshare.rs -B 3 -A 5Repository: gnosisguild/enclave
Length of output: 45
🏁 Script executed:
# Search for ComputeRequestError across the entire codebase
rg -n 'ComputeRequestError' crates/ --type rust | head -20Repository: gnosisguild/enclave
Length of output: 2620
🏁 Script executed:
# Check if ThresholdKeyshare subscribes to ComputeRequestError
rg -n 'subscribe.*ComputeRequestError|ComputeRequestError' crates/keyshare/src/threshold_keyshare.rsRepository: gnosisguild/enclave
Length of output: 45
Verify the ThresholdKeyshare-side E3Failed routing for computation stage failures.
The documentation claims E3Failed emission from ThresholdKeyshare for GenPkShareAndSkSss, GenEsiSss, CalculateDecryptionKey, and CalculateDecryptionShare errors, but these handlers are not wired to emit E3Failed in the code.
Verification confirms that:
EncryptionKeyCollectionFailedandThresholdShareCollectionFailedhandlers exist (lines 2471, 2499) and do emit E3Failed (line 2562) ✓- The four computation response handlers exist but use
?operator to propagate errors into thetrap()function, which converts them toEnclaveErrorevents instead ofE3Failed ThresholdKeysharedoes not subscribe toComputeRequestErrorevents, so computation failures don't trigger the documented E3Failed path
The doc is ahead of the code for computation stage failures. Either implement the E3Failed handlers for these stages or update the documentation to reflect that computation errors become EnclaveError events, not E3Failed.
Also applies to: 114-121, 174-182, 188-194, 323-330, 688-694
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md` around lines 47 - 52, The doc
claims computation-stage failures from ThresholdKeyshare (GenPkShareAndSkSss,
GenEsiSss, CalculateDecryptionKey, CalculateDecryptionShare) emit E3Failed, but
the code currently propagates errors via the ? operator into trap() producing
EnclaveError and ThresholdKeyshare also doesn't subscribe to
ComputeRequestError; fix by wiring these computation-response handlers to
explicitly emit E3Failed events instead of using ? into trap() and ensure
ThresholdKeyshare subscribes to ComputeRequestError; update the four handler
implementations (functions handling GenPkShareAndSkSss, GenEsiSss,
CalculateDecryptionKey, CalculateDecryptionShare) to convert errors into the
E3Failed emission path, and add subscription for ComputeRequestError in the
ThresholdKeyshare setup so computation failures trigger the E3Failed path rather
than EnclaveError.
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.
close #1536
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation