refactor: access multithread actor exclusively via bus [skip-line-limit]#1146
Conversation
📝 WalkthroughWalkthroughThis PR centralizes async compute handling: it converts compute events to struct-based requests/responses with correlation IDs, introduces a TaskPool-backed Multithread actor and reporting, and removes direct Multithread dependencies from aggregator/keyshare extensions while switching to bus-published compute flows and trap-based orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Event Handler
participant Bus as Event Bus
participant Multithread as Multithread Actor
participant TaskPool as TaskPool
participant Compute as TrBFV Compute
participant Report as MultithreadReport
participant Aggregator as ThresholdPlaintextAggregator
Handler->>Bus: publish(ComputeRequest{request, correlation_id, e3_id})
Bus->>Multithread: EnclaveEvent(ComputeRequest)
Multithread->>TaskPool: spawn(handle_compute_request_event)
TaskPool->>Compute: execute specific TrBFV operation
Compute-->>TaskPool: result / error
alt success
Multithread->>Report: TrackDuration(name,duration)
Multithread->>Bus: publish(ComputeResponse{response, correlation_id, e3_id})
else error
Multithread->>Bus: publish(ComputeRequestError{kind, request})
end
Bus->>Aggregator: EnclaveEvent(ComputeResponse)
Aggregator->>Aggregator: handle_compute_response()
Aggregator->>Bus: publish(PlaintextAggregated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ryardley
changed the title
ryardley
changed the title
ryardley
changed the title
ryardley
force-pushed
the
ry/1050-events
branch
from
d1bb588 to
fbbace5
Compare
…side from drop them on the bus which we are doing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/multithread/src/report.rs (1)
70-79: Consider avoiding redundant clones when calculating averages.The
total_dur.clone()on line 71 could be avoided by iterating overtotal_dur.iter()or by restructuring to consume the map only once at the end.🔎 Suggested improvement
- let avg_dur = total_dur - .clone() - .into_iter() + let avg_dur = total_dur + .iter() .map(|(name, total)| { - let count = runs[&name]; - let avg = Duration::from_nanos((total.as_nanos() / count as u128) as u64); - (name, avg) + let count = runs[name]; + let avg = Duration::from_nanos((total.as_nanos() / count as u128) as u64); + (name.clone(), avg) }) .collect();crates/multithread/src/multithread.rs (1)
108-124: Consider propagating errors instead of just logging them.The handler logs errors on line 120 but returns
(). While thebus.err()call inhandle_compute_request_event(line 153) properly publishes errors for compute failures, any errors fromhandle_compute_request_eventitself (e.g., pool spawn failures, publish failures) are only logged.The TODO on line 116 suggests this will be addressed with
trap_futin the future.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
crates/aggregator/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/Cargo.tomlcrates/ciphernode-builder/src/ciphernode_builder.rscrates/entrypoint/src/start/aggregator_start.rscrates/entrypoint/src/start/start.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/enclave_error.rscrates/events/src/enclave_event/mod.rscrates/keyshare/src/ext.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/Cargo.tomlcrates/multithread/src/lib.rscrates/multithread/src/multithread.rscrates/multithread/src/pool.rscrates/multithread/src/report.rscrates/tests/tests/integration.rs
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2025-04-30T06:25:14.721Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 372
File: packages/ciphernode/events/src/eventbus.rs:15-15
Timestamp: 2025-04-30T06:25:14.721Z
Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.
Applied to files:
crates/events/src/enclave_event/enclave_error.rscrates/events/src/enclave_event/mod.rscrates/aggregator/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
📚 Learning: 2024-10-10T23:24:43.341Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 143
File: packages/ciphernode/sortition/src/sortition.rs:4-9
Timestamp: 2024-10-10T23:24:43.341Z
Learning: In the `Sortition` module (`packages/ciphernode/sortition/src/sortition.rs`), errors are sent to the event bus using `self.bus.err`, which handles logging and printing. Therefore, explicit use of the `tracing` crate for logging errors may not be necessary in this context.
Applied to files:
crates/events/src/enclave_event/enclave_error.rscrates/aggregator/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
crates/ciphernode-builder/Cargo.tomlcrates/multithread/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
crates/ciphernode-builder/Cargo.toml
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/ciphernode-builder/Cargo.tomlcrates/entrypoint/src/start/start.rscrates/entrypoint/src/start/aggregator_start.rscrates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
crates/entrypoint/src/start/start.rscrates/entrypoint/src/start/aggregator_start.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/entrypoint/src/start/start.rscrates/entrypoint/src/start/aggregator_start.rs
📚 Learning: 2024-10-28T12:00:09.010Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave/src/commands/wallet/set.rs:17-23
Timestamp: 2024-10-28T12:00:09.010Z
Learning: In the `enclave` package of the `ciphernode` project, prefer using `println!` over logging macros like `error!` from the `tracing` crate for error output in CLI commands.
Applied to files:
crates/entrypoint/src/start/start.rscrates/entrypoint/src/start/aggregator_start.rs
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
crates/entrypoint/src/start/start.rscrates/ciphernode-builder/src/ciphernode_builder.rs
📚 Learning: 2024-10-28T12:04:26.578Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/enclave_node/src/ciphernode.rs:26-28
Timestamp: 2024-10-28T12:04:26.578Z
Learning: In the `setup_ciphernode` function in `packages/ciphernode/enclave_node/src/ciphernode.rs`, panicking on errors during setup is acceptable.
Applied to files:
crates/entrypoint/src/start/start.rscrates/entrypoint/src/start/aggregator_start.rscrates/tests/tests/integration.rs
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
crates/entrypoint/src/start/start.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/keyshare/src/ext.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
crates/entrypoint/src/start/aggregator_start.rscrates/tests/tests/integration.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T07:15:06.690Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/evm/src/ciphernode_registry_sol.rs:133-143
Timestamp: 2024-10-08T07:15:06.690Z
Learning: In `packages/ciphernode/evm/src/ciphernode_registry_sol.rs`, the function `helpers::stream_from_evm` in Rust returns `()`, not a `Result`, so error handling with `if let Err(e) = ...` is not applicable.
Applied to files:
crates/entrypoint/src/start/aggregator_start.rs
📚 Learning: 2024-10-22T03:17:41.617Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/hooks.rs:0-0
Timestamp: 2024-10-22T03:17:41.617Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, in the `hydrate` methods of the `E3Feature` implementations, it's acceptable to return `Ok(())` when dependencies are missing, as the error is reported to the bus.
Applied to files:
crates/entrypoint/src/start/aggregator_start.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
📚 Learning: 2025-11-14T21:25:06.925Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1016
File: crates/multithread/src/lib.rs:44-82
Timestamp: 2025-11-14T21:25:06.925Z
Learning: In crates/multithread/src/lib.rs, the Multithread::new constructor must ensure that Rayon's global thread pool is configured with exactly 1 thread to maintain actor model constraints. The panic on misconfiguration (>1 threads) is intentional and by design.
Applied to files:
crates/tests/tests/integration.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/multithread/src/pool.rscrates/multithread/src/lib.rscrates/multithread/src/multithread.rscrates/multithread/src/report.rs
📚 Learning: 2024-10-16T09:52:53.807Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/src/data_store.rs:74-75
Timestamp: 2024-10-16T09:52:53.807Z
Learning: In this project, the actor model handles potential errors internally, so methods like `checkpoint` in the `Checkpoint` trait do not need to explicitly handle or propagate errors.
Applied to files:
crates/tests/tests/integration.rscrates/keyshare/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
📚 Learning: 2024-11-25T09:56:11.195Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/src/network_relay.rs:0-0
Timestamp: 2024-11-25T09:56:11.195Z
Learning: In `NetworkPeer::new` in `packages/ciphernode/net/src/network_peer.rs`, peers are converted to multiaddrs, and validation of the address format occurs during this conversion. Duplicate addresses are managed as connections are deduplicated.
Applied to files:
crates/tests/tests/integration.rs
📚 Learning: 2025-10-04T14:00:17.916Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 660
File: crates/events/src/enclave_event/decryptionshare_created.rs:23-26
Timestamp: 2025-10-04T14:00:17.916Z
Learning: In the enclave codebase using threshold BFV cryptography, decryption shares (as in DecryptionshareCreated events) are not sensitive data and can be safely logged or displayed, as individual shares reveal nothing without reaching the threshold number of shares.
Applied to files:
crates/tests/tests/integration.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-29T01:04:19.137Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:53-70
Timestamp: 2024-10-29T01:04:19.137Z
Learning: In the `Keyshare` struct within `packages/ciphernode/keyshare/src/keyshare.rs`, the `self.secret` field is stored in encrypted form and is not considered sensitive.
Applied to files:
crates/keyshare/src/ext.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
crates/keyshare/src/ext.rs
📚 Learning: 2024-11-05T06:51:46.701Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/evm/src/event_reader.rs:270-286
Timestamp: 2024-11-05T06:51:46.701Z
Learning: In `packages/ciphernode/evm/src/event_reader.rs`, the `state.ids` HashSet is intended to grow indefinitely to store all processed event IDs for deduplication purposes until an alternative like a bloom filter is implemented.
Applied to files:
crates/aggregator/src/threshold_plaintext_aggregator.rs
📚 Learning: 2025-11-08T01:31:47.505Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.505Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-05-07T15:18:20.056Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 388
File: packages/commons/src/bfv/mod.rs:38-48
Timestamp: 2025-05-07T15:18:20.056Z
Learning: The `build_bfv_params` and related functions in the commons BFV utilities intentionally use panic for error handling as a temporary solution. The team plans to refactor these to use proper Result-based error handling in the future.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (4)
crates/tests/tests/integration.rs (3)
crates/multithread/src/multithread.rs (2)
create_taskpool(88-90)new(51-65)crates/multithread/src/pool.rs (2)
new(34-44)new(118-120)crates/multithread/src/report.rs (2)
new(23-25)new(40-46)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
crates/multithread/src/multithread.rs (2)
create_taskpool(88-90)attach(76-86)
crates/aggregator/src/threshold_plaintext_aggregator.rs (3)
crates/events/src/enclave_event/enclave_error.rs (2)
trap(65-73)new(46-51)crates/trbfv/src/calculate_threshold_decryption.rs (1)
calculate_threshold_decryption(126-166)crates/events/src/enclave_event/compute_request/mod.rs (3)
new(35-45)new(73-83)new(96-98)
crates/events/src/enclave_event/compute_request/mod.rs (1)
crates/events/src/correlation_id.rs (1)
fmt(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_crisp_sdk
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
🔇 Additional comments (33)
crates/ciphernode-builder/Cargo.toml (1)
28-28: No issues found. Rayon is properly imported and used in ciphernode-builder (ThreadPool at line 39), correctly defined in the workspace Cargo.toml (version 1.10.0), and appropriately added as a workspace dependency.crates/multithread/Cargo.toml (1)
21-21: LGTM!The
thiserrordependency addition is appropriate for the newTaskPoolErrorenum inpool.rs.crates/multithread/src/pool.rs (3)
32-44: LGTM!The
TaskPool::newconstructor correctly initializes both the Rayon thread pool and Tokio semaphore. Theexpect()on thread pool creation is acceptable for initialization-time failures. Based on learnings, panic on misconfiguration is intentional and by design.
79-98: LGTM!The oneshot channel pattern correctly bridges the sync Rayon execution with async callers, and aborting the warning handle after completion prevents spurious logs.
101-136: LGTM!The
TaskTimeoutsandTimedLogtypes provide a clean API for configuring timed logging with sensible defaults and ergonomicFromimplementations.crates/events/src/enclave_event/enclave_error.rs (1)
42-42: LGTM!The
Computationvariant extendsETypeto support error categorization for the new compute request/response flow.crates/entrypoint/src/start/start.rs (1)
9-9: LGTM!Import cleanup removes unused
get_enclave_bus_handleandget_enclave_event_bussymbols, reducing coupling to internal builder helpers.crates/entrypoint/src/start/aggregator_start.rs (1)
8-8: LGTM!Consistent with the import cleanup in
start.rs, removing unused builder helper imports.crates/events/src/enclave_event/mod.rs (3)
125-127: LGTM!The new
ComputeRequest,ComputeResponse, andComputeRequestErrorvariants properly extendEnclaveEventDatato support the bus-driven compute flow.
343-346: LGTM!The
impl_into_event_datamacro correctly includes all three new compute-related types, enabling seamless conversion toEnclaveEventData.
308-308: AddComputeRequestvariant toget_e3_id.
ComputeRequestis anEnclaveEventDatavariant with ane3_idfield but is missing from theget_e3_idmatch statement. It should be handled alongsideComputeResponsefor consistency.⛔ Skipped due to learnings
Learnt from: ryardley Repo: gnosisguild/enclave PR: 139 File: packages/ciphernode/core/src/events.rs:323-323 Timestamp: 2024-10-08T01:50:45.185Z Learning: When suggesting that instances of `E3Requested` should include `src_chain_id`, double-check to ensure that the field is actually missing before making the suggestion.Learnt from: ryardley Repo: gnosisguild/enclave PR: 372 File: packages/ciphernode/events/src/eventbus.rs:15-15 Timestamp: 2025-04-30T06:25:14.721Z Learning: EnclaveEvent implements Display in packages/ciphernode/events/src/enclave_event/mod.rs, which satisfies the Event trait requirement. Static analysis tools may incorrectly flag implementations as missing when they do exist.crates/tests/tests/integration.rs (7)
7-7: LGTM!Added
Actorimport for the.start()call onMultithreadReport.
17-17: LGTM!Updated imports reflect the new API:
MultithreadReportandToReportreplace the previousGetReportpattern.
170-171: LGTM!Shared
task_poolandmultithread_reportare created once and wired into all ciphernode builders, enabling centralized metrics collection across simulated nodes.
181-183: LGTM!The collector node correctly receives the shared task pool and report handle via the new builder methods.
198-200: LGTM!Normal nodes also receive the shared resources, ensuring all nodes contribute to the same task pool and metrics reporter.
395-399: LGTM!The expected event sequence correctly includes the new
ComputeRequestandComputeResponseevents that are now part of the bus-driven compute flow.
443-444: LGTM!Report retrieval correctly uses the new
ToReportmessage sent to the sharedmultithread_reportactor.crates/multithread/src/lib.rs (1)
7-13: LGTM!Clean module reorganization separates concerns into dedicated submodules (
multithread,pool,report) while maintaining API compatibility through glob re-exports.crates/keyshare/src/ext.rs (1)
114-135: Clean removal of Multithread dependency from ThresholdKeyshareExtension.The struct and constructor changes properly remove the
multithread: Addr<Multithread>field and parameter. The extension now relies on bus-driven compute requests, which aligns with the PR's architectural goal of decoupling domain actors from direct Multithread access.crates/aggregator/src/ext.rs (1)
248-260: Clean removal of Multithread dependency from ThresholdPlaintextAggregatorExtension.The simplified
create()signature and struct properly remove the Multithread reference. The aggregator will now receive compute results viaComputeResponseevents on the bus instead of direct actor communication.crates/multithread/src/report.rs (1)
35-37: LGTM - MultithreadReport correctly implements Actor trait.The conversion to an Actix actor with proper message handlers (
TrackDuration,ToReport) provides clean encapsulation and enables multiple Multithread actors to share a single reporter instance as intended by the PR.crates/aggregator/src/threshold_plaintext_aggregator.rs (2)
227-254: Verify correlation_id matching for compute responses.The
handle_compute_responsecorrectly filters bye3_id(line 229), but thecorrelation_idcreated inhandle_compute_aggregate(line 220) is not stored or verified. If multiple compute requests can be in-flight for the samee3_id, responses might be misattributed.Currently, the code filters by response type (
TrBFVResponse::CalculateThresholdDecryption), which may be sufficient if only one such request is active per E3. Please verify this assumption holds.
335-351: LGTM - trap-based error handling for compute flows.The
trap()wrapper correctly routes errors to the bus viaEType::PlaintextAggregation, maintaining consistent error handling throughout the aggregator. The handler implementations are clean and follow the established pattern.crates/ciphernode-builder/src/ciphernode_builder.rs (2)
221-231: Good additions for sharing TaskPool and MultithreadReport across ciphernodes.The
with_shared_taskpoolandwith_shared_multithread_reportbuilder methods enable test scenarios where multiple simulated nodes share a single thread pool and report aggregator, as described in the PR objectives.
530-560: LGTM - ensure_multithread correctly wires shared components.The refactored method properly:
- Returns cached instance if available
- Creates TaskPool lazily if not provided externally
- Passes the bus, rng, cipher, task_pool, and optional report to
Multithread::attachThis enables both standalone operation and shared resource scenarios.
crates/multithread/src/multithread.rs (2)
76-90: LGTM - Multithread::attach correctly subscribes to ComputeRequest events.The
attachmethod properly creates the actor, starts it, and subscribes to"ComputeRequest"events on the bus. Thecreate_taskpoolassociated function provides a clean factory method for TaskPool creation.
174-269: Well-structured compute request dispatch with consistent error handling.Each TrBFV request variant is handled uniformly with:
- Timing via
timefunc- Success →
ComputeResponse::newwith correlation_id and e3_id- Failure →
ComputeRequestError::newwith typedTrBFVErrorThe pattern is consistent and maintainable.
crates/events/src/enclave_event/compute_request/mod.rs (2)
24-46: Well-designed struct-based ComputeRequest with correlation tracking.The new struct design with
correlation_idande3_idfields enables:
- Request-response correlation across async flows
- E3-scoped filtering for compute responses
- Future extensibility (as noted in the comment about
e3_idpotentially becoming optional)The
#[rtype(result = "()")]change aligns with the bus-driven publish pattern.
86-110: ComputeRequestError retains original request for debugging.Storing the original
requestinComputeRequestErroris a good practice - it enables:
- Better error diagnostics (knowing which request failed)
- Potential retry logic if needed
- Correlation tracking via the embedded request's correlation_id
crates/keyshare/src/threshold_keyshare.rs (3)
409-423: Well-structured compute response dispatcher.The centralized
handle_compute_responsemethod cleanly routes responses to type-specific handlers based on theTrBFVResponsevariant. ReturningOk(())for unhandled types (line 421) is correct behavior since the keyshare actor may receive compute responses intended for other actors.
955-970: LGTM - Consistent trap-based error handling.The handlers for
ComputeResponseandCiphernodeSelectedcorrectly usetrap(EType::KeyGeneration, ...)to propagate errors to the bus. This maintains consistency with the error handling pattern established elsewhere in the codebase.
522-537: Verify that concurrent compute requests won't cause response misattribution.Both
handle_gen_esi_sss_requestedandhandle_gen_pk_share_and_sk_sss_requestedcreate independentCorrelationIdinstances but don't track them for verification when responses arrive. Since both requests can be in-flight simultaneously (lines 493-496 inhandle_all_encryption_keys_collected), responses are distinguished only by theirTrBFVResponsevariant type.This works correctly because each variant (
GenEsiSss,GenPkShareAndSkSss) is distinct, but the correlation_id is effectively unused for matching. Consider documenting this design decision or utilizing the correlation_id for verification if multiple requests of the same type could ever be concurrent.Also applies to: 606-616
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/multithread/src/pool.rs (2)
93-96: Update error message terminology for consistency.The error message references "multithread actor" but this component is now the TaskPool. Consider updating for clarity.
🔎 Proposed terminology update
if let Err(res) = tx.send(t) { error!( - "There was an error sending the result from the multithread actor: result = {:?}", + "There was an error sending the result from the task pool: result = {:?}", res ); }
100-104: Consider aborting warning_handle on all exit paths.If
rx.awaitfails (e.g., due to a panic in the task), the warning_handle continues running and may log errors for an already-failed task. While the task will eventually complete on its own, aborting it on the error path would prevent confusing log output and conserve resources.🔎 Proposed cleanup on error path
- let output = rx.await.map_err(|r| TaskPoolError::RecvError(r))?; - - warning_handle.abort(); - - Ok(output) + let result = rx.await.map_err(|r| TaskPoolError::RecvError(r)); + + warning_handle.abort(); + + result
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/multithread/src/pool.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-14T21:25:06.925Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 1016
File: crates/multithread/src/lib.rs:44-82
Timestamp: 2025-11-14T21:25:06.925Z
Learning: In crates/multithread/src/lib.rs, the Multithread::new constructor must ensure that Rayon's global thread pool is configured with exactly 1 thread to maintain actor model constraints. The panic on misconfiguration (>1 threads) is intentional and by design.
Applied to files:
crates/multithread/src/pool.rs
📚 Learning: 2024-11-05T03:56:22.254Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 170
File: packages/ciphernode/evm/tests/evm_reader.rs:63-63
Timestamp: 2024-11-05T03:56:22.254Z
Learning: In the Rust test function `test_logs` in `packages/ciphernode/evm/tests/evm_reader.rs`, a sleep duration of 1 millisecond is sufficient for reliable event processing, even in CI environments.
Applied to files:
crates/multithread/src/pool.rs
📚 Learning: 2025-08-02T15:41:12.872Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 609
File: crates/crypto/src/secret_holder.rs:110-112
Timestamp: 2025-08-02T15:41:12.872Z
Learning: In `crates/crypto/src/secret_holder.rs`, the `TimedSecretHolder` uses `try_lock()` instead of `lock()` in the purge callback intentionally. This creates graceful behavior where purging only happens when the secret is idle (lock available) and skips purging when the secret is actively being used (lock held), preventing blocking and deadlocks while maintaining security.
Applied to files:
crates/multithread/src/pool.rs
🧬 Code graph analysis (1)
crates/multithread/src/pool.rs (1)
crates/multithread/src/multithread.rs (1)
new(51-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_sdk
- GitHub Check: build_crisp_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: test_net
🔇 Additional comments (1)
crates/multithread/src/pool.rs (1)
64-84: Timeout implementation correctly uses absolute thresholds.The elapsed-time tracking at lines 65-74 ensures that each timeout threshold is measured from task start, not cumulatively. This resolves the concern raised in the previous review.
hmzakhalid
left a comment
hmzakhalid
left a comment
There was a problem hiding this comment.
Amazing Work!
Just a few comments
2 participants
Moves forward with #1050 specifically this comment
This severs the dependency between multithread and core.
When domain actors want to run jobs they dispatch events to the
BusHandle. TheMultithreadactor wraps a ThreadPool that holds a semaphore limiting the concurrent jobs to avoid threadstarvation.This PR extracts the reporting to it's own actor which enables us to test multiple
Multithreadactors for several simulated nodes that all send metrics to a single reporter.Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.