feat: add share encryption [skip-line-limit]#1114
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds BFV-based share encryption and BFV key-exchange: new Changes
Sequence Diagram(s)sequenceDiagram
participant Party as Party / Ciphernode
participant Net as Network / DocumentPublisher
participant TKS as ThresholdKeyshare
participant EKC as EncryptionKeyCollector
rect rgb(230,240,255)
Note over Party,TKS: Phase 1 — BFV key generation & publish
Party->>TKS: ciphernode_selected (trigger)
TKS->>TKS: generate BFV keys (sk_bfv, pk_bfv)
TKS->>EKC: setup(parent=TKS, total)
TKS->>Net: Publish EncryptionKeyCreated(e3_id, pk_bfv, party_id)
Net->>Party: broadcast EncryptionKeyCreated
end
rect rgb(220,255,235)
Note over Party,EKC: Phase 2 — Key collection
Party->>EKC: EncryptionKeyCreated (per party)
EKC->>EKC: validate & store by party_id
EKC->>TKS: AllEncryptionKeysCollected(keys)
end
rect rgb(255,250,210)
Note over TKS: Phase 3 — Share generation & BFV encryption
TKS->>TKS: transition → GeneratingThresholdShare
TKS->>TKS: generate Shamir shares
TKS->>TKS: encrypt shares with collected pk_bfv
TKS->>Net: Publish ThresholdShareCreated(BFV-encrypted)
Net->>Party: deliver encrypted shares
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
hmzakhalid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/trbfv/src/helpers.rs (1)
46-50: Consider making encryption parameters configurable to support different security levels.The hardcoded parameter set (Set8192_144115188075855872_2) works for now, but as indicated by the TODO, configurability would enable flexibility for different security requirements or use cases. BFV parameters affect many aspects of the encryption scheme, such as operational performance, security level, multiplicative depth of the circuit, and space consumption.
crates/trbfv/src/shares/bfv_encrypted.rs (1)
97-124: Consider validating degree against decrypted vector length.The
decryptmethod assumes the decrypted vector has at leastdegreeelements. If the plaintext was encoded with a different degree,take(degree)silently truncates or the loop underfillsdata. Consider adding a validation check.let decrypted: Vec<u64> = Vec::<u64>::try_decode(&pt, Encoding::poly()) .context("Failed to decode plaintext")?; + if decrypted.len() < degree { + anyhow::bail!( + "Decrypted vector has {} elements, expected at least {}", + decrypted.len(), + degree + ); + } + for (i, val) in decrypted.into_iter().take(degree).enumerate() { data[[m, i]] = val; }crates/keyshare/src/threshold_keyshare.rs (1)
909-910: Remove duplicate comment line.Line 909 and 910 contain the same comment.
-// Will only receive events that are for this specific e3_id // Will only receive events that are for this specific e3_id impl Handler<EnclaveEvent> for ThresholdKeyshare {
📜 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 (16)
crates/events/src/enclave_event/encryption_key_created.rs(1 hunks)crates/events/src/enclave_event/mod.rs(5 hunks)crates/events/src/enclave_event/threshold_share_created.rs(1 hunks)crates/keyshare/Cargo.toml(1 hunks)crates/keyshare/src/encryption_key_collector.rs(1 hunks)crates/keyshare/src/lib.rs(1 hunks)crates/keyshare/src/threshold_keyshare.rs(22 hunks)crates/net/src/document_publisher.rs(7 hunks)crates/test-helpers/src/usecase_helpers.rs(3 hunks)crates/tests/tests/integration.rs(1 hunks)crates/trbfv/Cargo.toml(1 hunks)crates/trbfv/src/helpers.rs(1 hunks)crates/trbfv/src/shares/bfv_encrypted.rs(1 hunks)crates/trbfv/src/shares/mod.rs(1 hunks)crates/trbfv/src/shares/pvw.rs(0 hunks)crates/trbfv/tests/integration.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/trbfv/src/shares/pvw.rs
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
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.
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.
📚 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/trbfv/Cargo.tomlcrates/keyshare/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/trbfv/Cargo.toml
📚 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/lib.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/encryption_key_created.rscrates/test-helpers/src/usecase_helpers.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/mod.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/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/keyshare/src/lib.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/encryption_key_created.rscrates/test-helpers/src/usecase_helpers.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/mod.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/tests/integration.rs
📚 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/keyshare/src/lib.rscrates/events/src/enclave_event/mod.rscrates/tests/tests/integration.rscrates/events/src/enclave_event/encryption_key_created.rscrates/net/src/document_publisher.rscrates/trbfv/src/shares/mod.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/keyshare/src/lib.rscrates/test-helpers/src/usecase_helpers.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/tests/integration.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/lib.rscrates/tests/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/tests/integration.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/keyshare/src/lib.rscrates/events/src/enclave_event/mod.rscrates/tests/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/mod.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/tests/integration.rs
📚 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/mod.rscrates/events/src/enclave_event/encryption_key_created.rscrates/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.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/test-helpers/src/usecase_helpers.rscrates/trbfv/src/helpers.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/trbfv/tests/integration.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/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.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/net/src/document_publisher.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/trbfv/src/shares/mod.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/trbfv/src/shares/bfv_encrypted.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (7)
crates/tests/tests/integration.rs (1)
crates/net/src/bin/p2p_test.rs (1)
report(111-114)
crates/events/src/enclave_event/encryption_key_created.rs (2)
crates/events/src/enclave_event/mod.rs (1)
fmt(352-354)crates/events/src/enclave_event/threshold_share_created.rs (1)
fmt(46-48)
crates/test-helpers/src/usecase_helpers.rs (2)
crates/trbfv/src/helpers.rs (1)
get_share_encryption_params(47-50)crates/trbfv/src/shares/bfv_encrypted.rs (1)
encrypt_all(154-175)
crates/net/src/document_publisher.rs (3)
crates/events/src/enclave_event/mod.rs (1)
get_e3_id(280-300)crates/keyshare/src/threshold_keyshare.rs (11)
get_e3_id(218-220)handle_encryption_key_created(381-390)msg(716-726)msg(729-743)handle(913-925)handle(930-935)handle(940-945)handle(950-955)handle(960-965)handle(970-975)handle(980-985)crates/keyshare/src/encryption_key_collector.rs (1)
handle(93-130)
crates/keyshare/src/encryption_key_collector.rs (1)
crates/keyshare/src/threshold_keyshare.rs (1)
from(67-71)
crates/events/src/enclave_event/threshold_share_created.rs (2)
crates/test-helpers/src/usecase_helpers.rs (2)
shares(156-165)shares(168-181)crates/utils/src/utility_types.rs (1)
fmt(47-49)
crates/trbfv/tests/integration.rs (2)
crates/test-helpers/src/usecase_helpers.rs (5)
generate_shares_hash_map(40-120)get_public_key(122-138)shares(156-165)shares(168-181)get_decryption_keys(140-200)crates/utils/src/helpers.rs (1)
to_ordered_vec(8-20)
⏰ 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_enclave_cli
- GitHub Check: build_e3_support_dev
- GitHub Check: crisp_unit
- GitHub Check: build_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
🔇 Additional comments (46)
crates/keyshare/src/lib.rs (1)
7-7: LGTM!The new
encryption_key_collectormodule declaration and public re-export follow the established pattern in this crate.Also applies to: 13-13
crates/tests/tests/integration.rs (1)
290-305: LGTM!The new synchronization point for
EncryptionKeyCreatedevents correctly integrates the BFV key exchange phase into the test flow. The event count (5) matchesthreshold_n, and the pattern is consistent with other event waits in this test.crates/trbfv/src/shares/mod.rs (1)
6-6: LGTM!The module structure change correctly introduces
bfv_encryptedto replace the PVW-based approach, aligning with the PR objective to encrypt Shamir shares using BFV.Also applies to: 9-9
crates/events/src/enclave_event/mod.rs (3)
22-22: LGTM!The
EncryptionKeyCreatedevent is properly integrated with module declaration, re-export, and enum variant addition.Also applies to: 51-51, 117-117
297-297: LGTM!E3 ID extraction correctly handles the new
EncryptionKeyCreatedvariant, maintaining consistency with other E3-scoped events.
329-330: LGTM!The
impl_into_event_data!macro invocation now includes bothThresholdShareCreatedandEncryptionKeyCreated, enabling theFromtrait conversions.crates/trbfv/src/helpers.rs (1)
26-44: LGTM!The
SecretKeyDatawrapper and serialization/deserialization functions provide a clean approach for persisting BFV secret keys via bincode. The pattern of cloning coefficients and reconstructing with parameters is appropriate sinceSecretKeydoesn't natively implementSerialize.crates/trbfv/tests/integration.rs (1)
86-103: LGTM!The test correctly adapts to the new
GeneratedSharesreturn type fromgenerate_shares_hash_map, properly accessinggenerated.sharesand passinggenerated.bfv_secret_keystoget_decryption_keys. This aligns with the BFV-based share encryption introduced in this PR.crates/events/src/enclave_event/encryption_key_created.rs (1)
17-37: LGTM!The new
EncryptionKeyandEncryptionKeyCreatedtypes follow the established patterns in the codebase. TheDisplayimplementation usingDebugformat is consistent with other event types (e.g.,ThresholdShareCreated), and the hex formatter forpk_bfvis appropriate for binary key data.crates/events/src/enclave_event/threshold_share_created.rs (1)
18-35: LGTM!The migration from PVW-based types to BFV-based
BfvEncryptedSharesaligns with the PR objective. The updated documentation accurately describes the BFV encryption semantics where each recipient decrypts only their intended share using their BFV secret key.crates/test-helpers/src/usecase_helpers.rs (4)
33-38: LGTM!The
GeneratedSharesstruct cleanly bundles the threshold shares with the BFV secret keys needed for decryption in tests. This separation keeps the test infrastructure organized while providing necessary key material.
50-61: LGTM!BFV keypair generation using
OsRngis appropriate for cryptographic key material. The loop generates a keypair per party, storing secret keys for later decryption and public keys for share encryption.
84-119: LGTM!The workflow correctly decrypts the locally stored secrets, then re-encrypts them using BFV for distribution to all recipients. Each party's share is encrypted with the corresponding recipient's BFV public key, enabling secure share distribution.
140-199: LGTM!The decryption flow correctly has each party use their BFV secret key to decrypt shares addressed to them. The
clone_share(party_id)method extracts the encrypted share for the specific recipient, anddecrypt()uses the corresponding secret key. This aligns with the BFV-encrypted share structure.crates/keyshare/src/encryption_key_collector.rs (3)
45-52: LGTM!The
Fromimplementation correctly sorts keys byparty_idfor deterministic ordering, which is essential for consistent cryptographic operations across all parties. This matches the pattern used inAllThresholdSharesCollected.
69-85: LGTM!The
setupfunction correctly initializes the collector with all expected party IDs (0 to total-1) in thetodoset. The actor starts immediately and returns its address for message routing.
91-130: LGTM!The handler correctly uses
HashSet::take()to atomically remove party IDs, naturally preventing duplicate key processing. The state machine transitions appropriately, anddo_sendis suitable for notifying the parent actor.crates/net/src/document_publisher.rs (7)
18-19: LGTM!The import additions for
EncryptionKeyCreatedare correctly placed alongside related event types.
73-81: LGTM!The
EncryptionKeyCreatedevent is correctly added tois_document_publisher_eventfollowing the same pattern asThresholdShareCreated. This ensures the event is properly routed through the document publishing system.
381-402: LGTM!The
ReceivableDocumentenum extension is well-structured. Theget_e3_idaccessor and serialization methods properly handle the new variant.
408-414: LGTM!Subscription to
EncryptionKeyCreatedevents is correctly added alongside the existing subscriptions.
433-450: LGTM!The
handle_encryption_key_createdmethod correctly mirrorshandle_threshold_share_created:
- Guards against external events to prevent republishing
- Wraps the message in
ReceivableDocument- Creates appropriate
DocumentMetawithDocumentKind::TrBFV- Publishes via
PublishDocumentRequested
451-472: LGTM!The
handle_document_receivedextension correctly handles theEncryptionKeyCreatedvariant, republishing it withexternal: trueto indicate it was received from the network.
479-509: LGTM!The
Handlerimplementations forEnclaveEventandEncryptionKeyCreatedcorrectly route events using the established patterns:
EnclaveEventhandler extracts and forwardsEncryptionKeyCreateddata- Dedicated handler invokes
handle_encryption_key_createdwith proper error loggingcrates/trbfv/src/shares/bfv_encrypted.rs (6)
1-26: LGTM!Clean module structure with appropriate license header, imports, and re-exports of helper functions from the parent module.
27-48: LGTM!Well-documented struct with clear purpose. The custom debug formatter for ciphertexts is a nice touch that avoids logging potentially large byte arrays while still providing useful summary information.
49-86: LGTM!The
encryptmethod correctly:
- Iterates over each modulus row of the Shamir share
- Encodes as polynomial plaintext
- Encrypts with the recipient's BFV public key
- Serializes ciphertexts for transmission
Error handling with
contextprovides clear diagnostics.
126-132: LGTM!Default implementation is appropriate for the type.
134-202: LGTM!
BfvEncryptedSharesprovides a clean API for managing encrypted shares:
encrypt_allproperly iterates parties and extracts per-party shares- Accessor methods (
get_share,clone_share,len,is_empty) follow Rust conventions- Default implementation is appropriate
204-258: LGTM!Tests provide good coverage of core functionality:
test_encrypt_decrypt_sharevalidates the roundtrip for encryption/decryptiontest_secret_key_serializationvalidates key serialization helpersThese tests ensure the fundamental cryptographic operations work correctly.
crates/keyshare/src/threshold_keyshare.rs (14)
74-79: LGTM!The
CollectingEncryptionKeysDatastruct correctly captures the BFV key pair and the originalCiphernodeSelectedevent needed for the next phase.
81-96: LGTM!Adding
sk_bfvandpk_bfvfields toGeneratingThresholdShareDataandAggregatingDecryptionKeyproperly carries the BFV key material through the state machine. UsingSensitiveBytesfor the secret key is appropriate.
112-128: LGTM!The state machine extension is well-designed:
- New
CollectingEncryptionKeysvariant inserted betweenInitandGeneratingThresholdShare- State transitions properly validated:
Init → CollectingEncryptionKeys → GeneratingThresholdSharevariant_nameupdated for debugging/loggingAlso applies to: 130-172
247-255: LGTM!
TryInto<CollectingEncryptionKeysData>implementation follows the established pattern for state extraction.
304-325: LGTM!New fields
encryption_key_collectorandcollected_encryption_keysproperly initialized toNonein the constructor.
351-390: LGTM!The
ensure_encryption_key_collectormethod correctly follows the pattern ofensure_collector, lazily initializing the collector when needed. Thehandle_encryption_key_createdmethod properly forwards events to the collector.
392-431: LGTM!The
handle_ciphernode_selectedmethod correctly:
- Initializes both collectors
- Generates a fresh BFV keypair using
OsRng- Encrypts the secret key for secure storage via
SensitiveBytes- Transitions state to
CollectingEncryptionKeys- Publishes
EncryptionKeyCreatedwithexternal: false
433-464: LGTM!The
handle_all_encryption_keys_collectedmethod properly:
- Stores collected keys for later use during encryption
- Extracts BFV keys from the current state
- Transitions to
GeneratingThresholdSharewith BFV key material- Triggers both ESI and PK share generation
498-546: LGTM!The
handle_gen_esi_sss_responsemethod is correctly updated to carrysk_bfvandpk_bfvthrough state transitions, with proper validation thatsk_bfvis present before transitioning toAggregatingDecryptionKey.
577-626: LGTM!The
handle_gen_pk_share_and_sk_sss_responsemethod correctly handles the new BFV key requirements:
- Validates that
sk_bfvis present whenesi_sssis available- Properly propagates BFV keys through state transitions
- Includes explicit error case for inconsistent state (
esi_ssspresent withoutsk_bfv)
628-691: LGTM!The
handle_shares_generatedmethod implements the core BFV encryption flow:
- Retrieves collected BFV public keys
- Deserializes public keys from bytes
- Decrypts local shares from encrypted storage
- Encrypts shares for all recipients using BFV
- Publishes
ThresholdShareCreatedwith BFV-encrypted sharesThis fulfills the PR objective of encrypting Shamir shares with BFV.
693-757: LGTM!The
handle_all_threshold_shares_collectedmethod correctly:
- Retrieves and decrypts the local BFV secret key
- Extracts the share meant for this party from each sender's encrypted shares
- Decrypts using BFV
- Re-encrypts for storage using the local cipher
- Builds the
CalculateDecryptionKeyRequestThe flow ensures each party can only decrypt shares intended for them.
911-926: LGTM!The
Handler<EnclaveEvent>correctly routesEncryptionKeyCreatedevents to the encryption key handler.
958-966: LGTM!The
Handler<AllEncryptionKeysCollected>implementation follows the established pattern for handling collected events, with proper error logging.crates/keyshare/Cargo.toml (1)
23-23: Confirm the workspaceCargo.tomldefines thefhedependency.The
fhe = { workspace = true }syntax is correct, but verify that the workspace rootCargo.tomlincludesfhein its[workspace.dependencies]section with an appropriate version before merging.crates/trbfv/Cargo.toml (1)
27-27: Verify rand usage in production code and ensure secure RNG for cryptographic operations.Moving
randfrom dev-dependencies to dependencies aligns with implementing BFV-based share encryption. However, please confirm:
- That
randis actually used in production code (not just tests)- That cryptographic operations use secure RNGs (
OsRng,ChaChaRng) rather than potentially insecure defaults likethread_rng()
0xjei
left a comment
0xjei
left a comment
There was a problem hiding this comment.
just gave a quick look - seems you are going in the right direction - lmk when ready for a proper review!
ryardley
left a comment
ryardley
left a comment
There was a problem hiding this comment.
Looks good. A couple of minor things I can see.
hmzakhalid
force-pushed
the
feat/share-encryption
branch
from
77a8d84 to
96acb34
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
439-450: Consider injecting BFV parameters as configuration.The
get_share_encryption_params()helper is called directly here and in other locations across the codebase. As noted in a previous review, centralizing parameter configuration would improve testability and flexibility.crates/trbfv/src/helpers.rs (1)
46-50: Make BFV parameters configurable.As noted in the TODO comment and previous review feedback, this hardcoded parameter set should be configurable to support different security levels and testing scenarios.
crates/trbfv/src/shares/bfv_encrypted.rs (1)
32-38: Addressed: UsingVec<ArcBytes>for debuggability and cloning.This addresses the past review comment about using
Vec<ArcBytes>for the ciphertexts field.crates/keyshare/src/threshold_keyshare.rs (2)
82-99: Fields changed from direct values to Option types.This addresses the past review comment about why fields are
Option. TheOptionwrapping allows these fields to be populated incrementally as the state machine progresses through different phases.
448-467: State extraction pattern matches past review suggestion.This addresses the past comment about using
let current: CollectingEncryptionKeysData = self.state.try_get()?.try_into()?;
🧹 Nitpick comments (4)
crates/events/src/chunk.rs (1)
53-60: Consider adding validation for chunk parameters.The
newconstructor doesn't validate that:
chunk_index < total_chunkstotal_chunks > 0Invalid chunk metadata could cause issues during reassembly.
🔎 Apply this diff to add validation:
pub fn new(chunk_id: ChunkSetId, chunk_index: u32, total_chunks: u32, data: Vec<u8>) -> Self { + assert!(total_chunks > 0, "total_chunks must be greater than 0"); + assert!(chunk_index < total_chunks, "chunk_index must be less than total_chunks"); Self { chunk_id, chunk_index, total_chunks, data, } }crates/keyshare/src/encryption_key_collector.rs (1)
105-111: Consider more idiomatic pattern for validation.The pattern
let Some(_) = self.todo.take(&pid)works but mixing pattern matching with side effects is less clear than alternatives.🔎 View suggested refactor:
-let Some(_) = self.todo.take(&pid) else { +if !self.todo.remove(&pid) { info!( "Error: {} was not in encryption key collector's ID list", pid ); return; };crates/net/src/chunking/collector.rs (1)
109-136: Consider logging the number of timed-out chunk sets for observability.The timeout cleanup logic is correct. However, if multiple chunk sets time out simultaneously, a single aggregate log message might be more useful for monitoring.
🔎 Optional improvement:
fn check_timeouts(&mut self) { let now = Instant::now(); let timed_out: Vec<_> = self .start_times .iter() .filter(|(_, start)| now.duration_since(**start) > self.timeout) .map(|(id, _)| id.clone()) .collect(); + if !timed_out.is_empty() { + info!("Cleaning up {} timed-out chunk sets", timed_out.len()); + } + for chunk_id in timed_out {crates/trbfv/src/shares/bfv_encrypted.rs (1)
188-193: Consider documenting the semantic change when usingextract_for_party.The
extract_for_partymethod creates a newBfvEncryptedShareswith only one share, but the resulting object will haveshares[0]contain the extracted share rather thanshares[party_id]. This could be confusing if the caller later usesget_share(party_id)on the extracted result.🔎 Suggested documentation:
/// Extract only the share for a specific party (for bandwidth optimization) + /// + /// Note: The returned `BfvEncryptedShares` contains only one share at index 0, + /// regardless of the original party_id. Use `get_share(0)` to access it. pub fn extract_for_party(&self, party_id: usize) -> Option<Self> {
📜 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 (28)
crates/ciphernode-builder/Cargo.toml(1 hunks)crates/ciphernode-builder/src/ciphernode_builder.rs(1 hunks)crates/events/Cargo.toml(1 hunks)crates/events/src/chunk.rs(1 hunks)crates/events/src/enclave_event/encryption_key_created.rs(1 hunks)crates/events/src/enclave_event/mod.rs(5 hunks)crates/events/src/enclave_event/publish_document/mod.rs(1 hunks)crates/events/src/enclave_event/threshold_share_created.rs(1 hunks)crates/events/src/lib.rs(2 hunks)crates/keyshare/Cargo.toml(1 hunks)crates/keyshare/src/encryption_key_collector.rs(1 hunks)crates/keyshare/src/ext.rs(4 hunks)crates/keyshare/src/lib.rs(1 hunks)crates/keyshare/src/threshold_keyshare.rs(24 hunks)crates/net/src/chunking/chunkable.rs(1 hunks)crates/net/src/chunking/collector.rs(1 hunks)crates/net/src/chunking/mod.rs(1 hunks)crates/net/src/document_publisher.rs(8 hunks)crates/net/src/lib.rs(1 hunks)crates/test-helpers/src/usecase_helpers.rs(3 hunks)crates/tests/tests/integration.rs(1 hunks)crates/trbfv/Cargo.toml(1 hunks)crates/trbfv/src/helpers.rs(1 hunks)crates/trbfv/src/shares/bfv_encrypted.rs(1 hunks)crates/trbfv/src/shares/mod.rs(1 hunks)crates/trbfv/src/shares/pvw.rs(0 hunks)crates/trbfv/tests/integration.rs(2 hunks)packages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.json(2 hunks)
💤 Files with no reviewable changes (1)
- crates/trbfv/src/shares/pvw.rs
✅ Files skipped from review due to trivial changes (1)
- packages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.json
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/trbfv/Cargo.toml
- crates/keyshare/src/lib.rs
- crates/keyshare/Cargo.toml
- crates/events/src/enclave_event/encryption_key_created.rs
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
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.
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.
📚 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/net/src/lib.rscrates/events/src/enclave_event/publish_document/mod.rscrates/events/src/lib.rscrates/ciphernode-builder/Cargo.tomlcrates/events/Cargo.tomlcrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/threshold_share_created.rscrates/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.rscrates/events/src/chunk.rs
📚 Learning: 2024-10-10T11:26:47.259Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 143
File: packages/ciphernode/p2p/src/libp2p_router.rs:154-154
Timestamp: 2024-10-10T11:26:47.259Z
Learning: In `packages/ciphernode/p2p/src/libp2p_router.rs`, logging the entire message data using `trace!("{:?}", message);` is acceptable at this point.
Applied to files:
crates/events/src/enclave_event/publish_document/mod.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/trbfv/src/helpers.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/events/src/enclave_event/mod.rscrates/trbfv/src/shares/mod.rscrates/keyshare/src/ext.rscrates/events/src/enclave_event/threshold_share_created.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/encryption_key_collector.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/trbfv/src/helpers.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/keyshare/src/ext.rscrates/events/src/enclave_event/threshold_share_created.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.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/trbfv/src/helpers.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/events/src/enclave_event/mod.rscrates/trbfv/src/shares/mod.rscrates/keyshare/src/ext.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.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/trbfv/src/helpers.rscrates/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/keyshare/src/ext.rscrates/tests/tests/integration.rscrates/events/src/enclave_event/threshold_share_created.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.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/lib.rscrates/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.rs
📚 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/lib.rscrates/events/src/enclave_event/mod.rscrates/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.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/events/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-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/ciphernode-builder/src/ciphernode_builder.rscrates/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/events/src/enclave_event/mod.rscrates/trbfv/src/shares/mod.rscrates/tests/tests/integration.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/encryption_key_collector.rs
📚 Learning: 2024-10-08T01:48:49.778Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
crates/ciphernode-builder/src/ciphernode_builder.rscrates/net/src/document_publisher.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/trbfv/tests/integration.rscrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/encryption_key_collector.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/trbfv/src/shares/mod.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/tests/tests/integration.rscrates/events/src/enclave_event/threshold_share_created.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/tests/tests/integration.rscrates/net/src/document_publisher.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/tests/tests/integration.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/net/src/document_publisher.rs
📚 Learning: 2025-05-12T21:12:33.453Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 376
File: packages/ciphernode/sortition/src/sortition.rs:155-164
Timestamp: 2025-05-12T21:12:33.453Z
Learning: When refactoring Rust code to work with collections like HashMaps and moving values into closures, the compiler can sometimes determine that a value won't be used again and allow moving fields out of a borrowed value without requiring an explicit `move` keyword.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (10)
crates/ciphernode-builder/src/ciphernode_builder.rs (2)
crates/trbfv/src/helpers.rs (1)
get_share_encryption_params(47-50)crates/keyshare/src/ext.rs (2)
create(29-35)create(124-138)
crates/trbfv/tests/integration.rs (2)
crates/test-helpers/src/usecase_helpers.rs (5)
generate_shares_hash_map(40-120)get_public_key(122-138)shares(156-165)shares(168-181)get_decryption_keys(140-200)crates/utils/src/helpers.rs (1)
to_ordered_vec(8-20)
crates/test-helpers/src/usecase_helpers.rs (3)
crates/trbfv/src/helpers.rs (1)
get_share_encryption_params(47-50)crates/fhe/src/fhe.rs (1)
new(49-51)crates/trbfv/src/shares/bfv_encrypted.rs (1)
encrypt_all(155-176)
crates/keyshare/src/ext.rs (1)
crates/keyshare/src/threshold_keyshare.rs (2)
new(189-207)new(319-329)
crates/tests/tests/integration.rs (1)
crates/net/src/bin/p2p_test.rs (1)
report(111-114)
crates/events/src/enclave_event/threshold_share_created.rs (2)
crates/test-helpers/src/usecase_helpers.rs (3)
shares(156-165)shares(168-181)esi_sss(86-89)crates/trbfv/src/shares/bfv_encrypted.rs (1)
extract_for_party(189-193)
crates/net/src/chunking/collector.rs (2)
crates/events/src/chunk.rs (1)
new(53-60)crates/net/src/chunking/chunkable.rs (1)
from_chunks(58-101)
crates/trbfv/src/shares/bfv_encrypted.rs (1)
crates/trbfv/src/helpers.rs (3)
deserialize_secret_key(41-44)get_share_encryption_params(47-50)serialize_secret_key(33-38)
crates/keyshare/src/threshold_keyshare.rs (4)
crates/trbfv/src/helpers.rs (2)
deserialize_secret_key(41-44)serialize_secret_key(33-38)crates/keyshare/src/encryption_key_collector.rs (1)
setup(75-84)crates/utils/src/utility_types.rs (1)
from_bytes(25-27)crates/trbfv/src/shares/bfv_encrypted.rs (2)
len(196-198)encrypt_all(155-176)
crates/keyshare/src/encryption_key_collector.rs (1)
crates/keyshare/src/threshold_keyshare.rs (1)
from(68-72)
⏰ 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_sdk
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
- GitHub Check: build_ciphernode_image
- GitHub Check: build_crisp_sdk
- GitHub Check: test_net
🔇 Additional comments (36)
crates/ciphernode-builder/Cargo.toml (1)
25-25: LGTM!The
e3-trbfvworkspace dependency is correctly added in alphabetical order and follows the established pattern. This addition aligns with the PR's objective to implement BFV-based share encryption.crates/events/Cargo.toml (1)
21-21: The hex dependency addition is correct and necessary.The hex crate is already in use in the events crate source code (
crates/events/src/chunk.rs:34andcrates/events/src/seed.rs:36callhex::encode()), and the dependency is properly configured withhex = "=0.4.3"in the root workspace Cargo.toml. Thehex = { workspace = true }declaration in crates/events/Cargo.toml follows the correct pattern for workspace dependencies.crates/events/src/enclave_event/publish_document/mod.rs (1)
14-14: LGTM! Public re-export enables external Filter usage.The visibility change from private to public re-export allows external crates to construct
DocumentMetawith filters, which is necessary for the chunking and document publishing infrastructure introduced in this PR.crates/keyshare/src/encryption_key_collector.rs (1)
75-84: LGTM! Clean actor setup with proper initialization.The
setupmethod correctly initializes the collector with party IDs0..total, sets up the parent reference, and starts the actor. The design properly separates collection state from the parentThresholdKeyshareactor.crates/net/src/lib.rs (1)
7-7: LGTM! Public chunking module export.The new
chunkingmodule is properly exposed, enabling external crates to use the chunking infrastructure for large document transmission.crates/events/src/lib.rs (1)
8-8: LGTM! Chunk types properly exposed.The new
chunkmodule and its public re-export makeChunkSetIdandChunkedDocumentavailable at the crate root, following the existing pattern for other event types.Also applies to: 23-23
crates/tests/tests/integration.rs (1)
290-305: LGTM! Correct synchronization for BFV key exchange phase.The test now properly waits for all
EncryptionKeyCreatedevents before proceeding toThresholdShareCreatedevents, which correctly reflects the new BFV key exchange requirement. The timing instrumentation will help track the performance impact of this additional phase.crates/net/src/chunking/mod.rs (1)
1-12: LGTM! Clean module organization.The module properly aggregates chunking functionality, re-exporting the
Chunkabletrait,ChunkCollectoractor, and chunk types from both internal submodules ande3_events. This provides a clean public API surface for the chunking infrastructure.crates/trbfv/src/shares/mod.rs (1)
6-11: PVW module successfully removed; BFV encryption implementation complete.The replacement of
pvwwithbfv_encryptedis clean—no remaining PVW references or artifacts detected in the codebase. The architectural shift from PVW to BFV, which is based on the Learning with Errors (LWE) problem and was proposed by Fan and Vercauteren, aligns with the PR's stated objective to implement BFV-based share encryption.crates/trbfv/src/helpers.rs (1)
26-44: LGTM!The SecretKey serialization utilities are well-structured. The use of a simple wrapper struct and bincode for serialization is appropriate.
crates/net/src/chunking/chunkable.rs (2)
12-55: LGTM!The
Chunkabletrait andinto_chunksimplementation are well-designed. The content-based chunk ID generation ensures consistency, and the single-chunk optimization is a nice touch.
57-102: LGTM!The reassembly logic is robust with comprehensive validation:
- Empty chunk detection
- Chunk set consistency checks
- Duplicate index detection
- Proper ordering before concatenation
crates/keyshare/src/ext.rs (1)
115-220: LGTM!The
share_encryption_paramsfield is properly integrated throughout the extension lifecycle (create, on_event, hydrate). UsingArcfor the parameters is efficient for sharing.crates/events/src/enclave_event/mod.rs (1)
22-22: LGTM!The
EncryptionKeyCreatedevent is properly integrated with all necessary wiring points: module declaration, public export, enum variant,get_e3_idmatching, and conversion macro.Also applies to: 51-51, 117-117, 297-297, 329-330
crates/trbfv/tests/integration.rs (1)
86-103: LGTM!The test correctly adapts to the new
GeneratedSharesstructure, properly accessing bothsharesandbfv_secret_keysfields.crates/events/src/enclave_event/threshold_share_created.rs (2)
10-35: LGTM!The migration from PVW to BFV-encrypted shares is consistent and well-documented. Field types are updated appropriately.
37-58: LGTM!The helper methods are well-implemented:
extract_for_partycorrectly handles the case where shares might be missingnum_partiesprovides a convenient accessorcrates/test-helpers/src/usecase_helpers.rs (4)
33-38: LGTM!The
GeneratedSharesstruct cleanly separates the shares from the BFV secret keys, making it clear that the keys are provided for test decryption purposes.
47-62: LGTM!BFV key generation is properly implemented using
OsRngfor cryptographic randomness. Each party receives a unique key pair.
64-120: LGTM!The share generation and BFV encryption flow is correct:
- Generate shares with local encryption
- Decrypt locally to get plaintext shares
- Re-encrypt for all recipients using BFV public keys
This properly separates storage encryption from transport encryption.
140-199: LGTM!The decryption flow correctly uses party-specific BFV secret keys to decrypt shares. Error handling for missing shares is appropriate.
crates/net/src/chunking/collector.rs (3)
16-32: LGTM - Well-structured actor for chunk collection.The
ChunkCollectorstruct is cleanly designed with appropriate state tracking for chunk reassembly. The use ofPhantomData<T>for the generic type parameter is correct.
51-107: LGTM - Solid chunk handling logic with proper duplicate detection.The
handle_chunk_internalmethod correctly:
- Tracks expected totals and start times lazily
- Detects and ignores duplicate chunks by index
- Reassembles complete sets using
T::from_chunks- Cleans up state after successful reassembly
173-197: LGTM - Handler correctly delegates to internal logic and publishes results.The trait bounds on the
Handlerimpl properly constrainTto be publishable via the event bus.crates/trbfv/src/shares/bfv_encrypted.rs (3)
61-87: LGTM - Encryption logic correctly iterates over moduli rows.The encryption method properly:
- Iterates over each modulus row of the Shamir share
- Encodes as plaintext with poly encoding
- Encrypts with the recipient's BFV public key
- Wraps ciphertext bytes in
ArcBytes
98-124: LGTM - Decryption correctly reconstructs the share matrix.The decryption logic properly reverses the encryption process, reconstructing the 2D array from decrypted plaintexts.
217-246: LGTM - Comprehensive round-trip test for encryption/decryption.The test correctly validates that a share can be encrypted and decrypted back to its original value.
crates/net/src/document_publisher.rs (5)
34-38: LGTM - Reasonable chunk size for threshold shares.10MB max chunk size is appropriate for network transmission of large threshold shares.
391-437: LGTM - Clean integration of chunk collector with lazy initialization.The
ensure_chunk_collectormethod properly creates the collector on-demand when the first chunk is received.
458-498: LGTM - Well-structured party-filtered chunk publishing.The
publish_party_chunksmethod correctly handles both single-chunk (complete) documents and multi-chunk scenarios with appropriate logging.
500-534: LGTM - Threshold share publishing correctly extracts per-party shares.The logic properly:
- Skips external events to avoid re-publishing received shares
- Extracts shares for each party using
extract_for_party- Chunks large shares and publishes with party filtering
617-625: LGTM - Handler for EncryptionKeyCreated follows established pattern.Consistent with other handlers in the file.
crates/keyshare/src/threshold_keyshare.rs (4)
75-80: LGTM - Clean state struct for BFV key collection phase.The
CollectingEncryptionKeysDataproperly captures the BFV key pair and original event for continuation after collection completes.
396-435: LGTM - BFV key generation and state transition are correct.The handler properly:
- Generates BFV keypair using
OsRng- Encrypts the secret key with the node's cipher for secure storage
- Transitions to
CollectingEncryptionKeysstate- Publishes
EncryptionKeyCreatedevent for distribution
641-702: LGTM - BFV encryption of shares for all recipients.The
handle_shares_generatedmethod correctly:
- Retrieves collected BFV public keys from state
- Deserializes public keys from bytes
- Decrypts local shares using the node's cipher
- Encrypts shares for each recipient using their BFV public key
- Publishes the encrypted shares
706-768: LGTM - BFV decryption of received shares.The
handle_all_threshold_shares_collectedmethod correctly:
- Retrieves the node's BFV secret key from encrypted storage
- For each received share, extracts the portion encrypted for this party
- Decrypts using the node's BFV secret key
- Passes decrypted shares for aggregation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/keyshare/src/threshold_keyshare.rs (2)
518-539: Consider more defensive pattern matching for BFV key presence.While the current logic is correct (BFV keys are guaranteed to be
Somewhen enteringGeneratingThresholdShare), the pattern at line 530 uses wildcards(None, None, _, _)which would silently accept a corrupted state wheresk_bfvorcollected_encryption_keysisNone. For defensive programming, consider checking these explicitly.🔎 Apply this diff for more defensive pattern matching:
- let next = match (pk_share, sk_sss, &sk_bfv, &collected_encryption_keys) { + let next = match (pk_share, sk_sss, &sk_bfv, &pk_bfv, &collected_encryption_keys) { // If the other shares are here then transition to aggregation - (Some(pk_share), Some(sk_sss), Some(sk_bfv_ref), Some(keys)) => { + (Some(pk_share), Some(sk_sss), Some(sk_bfv_ref), Some(_), Some(keys)) => { K::AggregatingDecryptionKey(AggregatingDecryptionKey { esi_sss, pk_share, sk_sss, sk_bfv: sk_bfv_ref.clone(), collected_encryption_keys: keys.clone(), }) } // If the other shares are not here yet then dont transition - (None, None, _, _) => K::GeneratingThresholdShare(GeneratingThresholdShareData { + (None, None, Some(sk_bfv), Some(pk_bfv), Some(keys)) => K::GeneratingThresholdShare(GeneratingThresholdShareData { esi_sss: Some(esi_sss), pk_share: None, sk_sss: None, - sk_bfv, - pk_bfv, - collected_encryption_keys, + sk_bfv: Some(sk_bfv.clone()), + pk_bfv: Some(pk_bfv.clone()), + collected_encryption_keys: Some(keys.clone()), }), _ => bail!("Inconsistent state!"), };
598-626: Consider more defensive pattern matching for BFV key presence.Similar to the
handle_gen_esi_sss_responselogic, the pattern at line 610 uses(None, sk_bfv, _)which accepts any value forsk_bfvandcollected_encryption_keys. While the subsequent error cases (lines 621-625) catch some inconsistencies, it would be more defensive to explicitly validate that BFV keys are present when inGeneratingThresholdSharestate.🔎 Apply this diff for more defensive pattern matching:
- let next = match (esi_sss, sk_bfv, &collected_encryption_keys) { - // If the esi shares and BFV key are here then transition to aggregation - (Some(esi_sss), Some(sk_bfv), Some(keys)) => { + let next = match (esi_sss, sk_bfv, &pk_bfv, &collected_encryption_keys) { + // If esi shares and BFV keys are here then transition to aggregation + (Some(esi_sss), Some(sk_bfv), Some(_), Some(keys)) => { KeyshareState::AggregatingDecryptionKey(AggregatingDecryptionKey { esi_sss, pk_share, sk_sss, sk_bfv, collected_encryption_keys: keys.clone(), }) } - // If esi shares are not here yet then don't transition - (None, sk_bfv, _) => { + // If esi shares are not here yet but BFV keys are present, don't transition + (None, Some(sk_bfv), Some(pk_bfv), Some(keys)) => { KeyshareState::GeneratingThresholdShare(GeneratingThresholdShareData { esi_sss: None, pk_share: Some(pk_share), sk_sss: Some(sk_sss), - sk_bfv, - pk_bfv, - collected_encryption_keys, + sk_bfv: Some(sk_bfv), + pk_bfv: Some(pk_bfv.clone()), + collected_encryption_keys: Some(keys.clone()), }) } // If we have esi_sss but no sk_bfv, that's an error - (Some(_), None, _) => bail!("Have esi_sss but no sk_bfv - inconsistent state!"), + (Some(_), None, _, _) => bail!("Have esi_sss but no sk_bfv - inconsistent state!"), + // If we don't have esi_sss and missing BFV keys, that's an error too + (None, None, _, _) | (None, _, None, _) | (None, _, _, None) => { + bail!("Missing BFV keys in GeneratingThresholdShare state!") + } // If we have shares ready but no encryption keys, that's an error - (Some(_), Some(_), None) => { + (Some(_), Some(_), _, None) => { bail!("Have shares but no collected encryption keys - inconsistent state!") } };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/keyshare/src/threshold_keyshare.rs(23 hunks)crates/net/src/document_publisher.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
📚 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/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/net/src/document_publisher.rs
📚 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/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.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/net/src/document_publisher.rscrates/keyshare/src/threshold_keyshare.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/net/src/document_publisher.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/keyshare/src/threshold_keyshare.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: 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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/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/threshold_keyshare.rs
📚 Learning: 2025-05-12T21:12:33.453Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 376
File: packages/ciphernode/sortition/src/sortition.rs:155-164
Timestamp: 2025-05-12T21:12:33.453Z
Learning: When refactoring Rust code to work with collections like HashMaps and moving values into closures, the compiler can sometimes determine that a value won't be used again and allow moving fields out of a borrowed value without requiring an explicit `move` keyword.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (2)
crates/net/src/document_publisher.rs (5)
crates/net/src/chunking/chunkable.rs (2)
max_chunk_size(14-16)max_chunk_size(115-117)crates/events/src/enclave_event/publish_document/mod.rs (2)
new(45-59)new(84-87)crates/net/src/chunking/collector.rs (1)
setup(47-49)crates/events/src/enclave_event/mod.rs (1)
from_bytes(173-175)crates/events/src/enclave_event/threshold_share_created.rs (1)
num_parties(55-57)
crates/keyshare/src/threshold_keyshare.rs (4)
crates/trbfv/src/helpers.rs (2)
deserialize_secret_key(41-44)serialize_secret_key(33-38)crates/keyshare/src/encryption_key_collector.rs (1)
setup(75-84)crates/utils/src/utility_types.rs (1)
from_bytes(25-27)crates/trbfv/src/shares/bfv_encrypted.rs (2)
len(196-198)encrypt_all(155-176)
⏰ 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_ciphernode_image
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_enclave_circuits
- GitHub Check: build_enclave_cli
- GitHub Check: build_crisp_sdk
- GitHub Check: test_net
🔇 Additional comments (8)
crates/keyshare/src/threshold_keyshare.rs (2)
640-701: LGTM! Secure share encryption implementation.The BFV encryption flow is correctly implemented:
- Public keys are deserialized with proper error handling
- Local shares are decrypted, encrypted for each recipient using their BFV public key, and then the plaintext is discarded
- Each party receives only their share, encrypted with their own public key
- Cryptographically secure RNG (OsRng) is used for encryption
706-767: LGTM! Correct BFV share decryption and aggregation.The decryption flow correctly:
- Retrieves and decrypts the BFV secret key from encrypted storage
- Extracts each party's share from the received encrypted collections
- Decrypts shares using the BFV secret key
- Re-encrypts for local compute operations
- Handles missing shares with appropriate error messages
Based on learnings, individual Shamir shares are not considered sensitive enough to require explicit zeroization in this codebase.
crates/net/src/document_publisher.rs (6)
34-38: LGTM! Appropriate chunk size for encrypted shares.Setting a 10 MB chunk size for
ThresholdShareCreatedis reasonable given that BFV-encrypted shares can be quite large, especially with multiple parties.
393-393: Excellent! Critical fix for per-E3ID chunk collection.This implementation correctly addresses the critical issue flagged in a previous review where chunks from different E3 IDs would be incorrectly routed to a single global collector. Now each E3 ID maintains its own
ChunkCollector, ensuring correct reassembly of multi-chunk documents.Also applies to: 429-434
436-495: LGTM! Well-structured party-filtered publishing with chunking support.The helper methods correctly:
- Apply party filtering to ensure each party only receives their specific shares
- Optimize single-chunk messages by publishing directly as
ThresholdShareCreated- Handle multi-chunk messages by publishing individual chunks for reassembly
- Include helpful logging for debugging
499-531: LGTM! Efficient party-filtered threshold share distribution.The implementation correctly:
- Prevents re-publishing of external shares
- Extracts per-party shares from the full threshold share
- Creates party-specific messages with appropriate chunking
- Distributes shares such that each party only receives their encrypted shares
This is more efficient than broadcasting all shares to all parties.
532-542: LGTM! Correct broadcast of encryption keys to all parties.BFV public keys are correctly broadcast to all parties (empty filter) since each party needs all other parties' public keys to encrypt shares for them. This is the expected behavior for the key exchange phase.
546-583: LGTM! Correct document-to-event conversion with chunk reassembly.The implementation correctly:
- Deserializes received documents to the appropriate event types
- Marks network-received events with
external: trueto prevent re-publishing- Routes chunks to the per-E3ID collector for reassembly
- Relies on upstream filtering (already done in
DocumentPublisher) to ensure only relevant documents are processed
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/net/src/document_publisher.rs (1)
34-38: Consider using the trait default instead of duplicating the value.The
Chunkabletrait already provides a defaultmax_chunk_size()of 10MB (per the relevant code snippet inchunking/chunkable.rs). This implementation duplicates that value.Suggested simplification:
-impl Chunkable for ThresholdShareCreated { - fn max_chunk_size() -> usize { - 10 * 1024 * 1024 - } -} +impl Chunkable for ThresholdShareCreated {}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/net/src/document_publisher.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/net/src/document_publisher.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/net/src/document_publisher.rs
📚 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/net/src/document_publisher.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/net/src/document_publisher.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/net/src/document_publisher.rs
🧬 Code graph analysis (1)
crates/net/src/document_publisher.rs (3)
crates/net/src/chunking/chunkable.rs (2)
max_chunk_size(14-16)max_chunk_size(115-117)crates/net/src/chunking/collector.rs (1)
setup(47-49)crates/events/src/enclave_event/threshold_share_created.rs (1)
num_parties(55-57)
⏰ 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: test_net
- GitHub Check: build_crisp_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
- GitHub Check: build_e3_support_dev
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
🔇 Additional comments (7)
crates/net/src/document_publisher.rs (7)
80-88: LGTM!Correctly extended to recognize
EncryptionKeyCreatedevents for document publishing.
391-394: Good fix for the chunk routing issue.The per-E3ID collector mapping correctly addresses the previous critical issue where chunks from different E3 IDs were routed to a single collector.
460-500: LGTM!The branching logic correctly handles both single complete chunks (direct publish) and multi-chunk cases (per-chunk publish). This optimization avoids chunk collection overhead for small documents.
502-536: LGTM!The party-filtered distribution logic correctly extracts per-party shares and publishes them with appropriate filters. Error handling for failed extractions is properly implemented.
537-547: LGTM!The empty filter
vec![]correctly broadcasts the encryption key to all interested parties, as opposed to the party-specific filtering used for threshold shares.
396-411: LGTM!The
ReceivableDocumentenum correctly extends to support the newEncryptionKeyCreatedandChunkvariants with appropriate serialization.
617-625: LGTM!Handler follows the established pattern used by other message handlers in this file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/events/src/enclave_event/threshold_share_created.rs (1)
55-57: Consider defensive validation for consistency.The method assumes all share collections have the same length, which is reasonable for well-formed instances. However, adding an assertion or debug check could catch malformed data during development.
🔎 Optional: Add debug assertion for consistency
pub fn num_parties(&self) -> usize { + debug_assert!( + self.esi_sss.iter().all(|shares| shares.len() == self.sk_sss.len()), + "All share collections should have the same length" + ); self.sk_sss.len() }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/events/src/enclave_event/threshold_share_created.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/events/src/enclave_event/threshold_share_created.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/events/src/enclave_event/threshold_share_created.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/events/src/enclave_event/threshold_share_created.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/events/src/enclave_event/threshold_share_created.rs
📚 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/events/src/enclave_event/threshold_share_created.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/events/src/enclave_event/threshold_share_created.rs
⏰ 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). (7)
- GitHub Check: crisp_unit
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
🔇 Additional comments (2)
crates/events/src/enclave_event/threshold_share_created.rs (2)
10-11: LGTM! Imports updated for BFV migration.The imports correctly bring in
BfvEncryptedSharesto replace the previous PVW-based types.
18-35: LGTM! ThresholdShare struct updated for BFV encryption.The struct definition correctly uses
BfvEncryptedSharestypes and the documentation clearly describes the BFV-based per-recipient encryption semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.rscrates/tests/tests/integration.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
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.
📚 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/tests/tests/integration.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/tests/tests/integration.rscrates/keyshare/src/threshold_keyshare.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/tests/tests/integration.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.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/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/tests/tests/integration.rscrates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.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/tests/tests/integration.rscrates/net/src/document_publisher.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/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.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/events/src/enclave_event/threshold_share_created.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-05-12T21:12:33.453Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 376
File: packages/ciphernode/sortition/src/sortition.rs:155-164
Timestamp: 2025-05-12T21:12:33.453Z
Learning: When refactoring Rust code to work with collections like HashMaps and moving values into closures, the compiler can sometimes determine that a value won't be used again and allow moving fields out of a borrowed value without requiring an explicit `move` keyword.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
📚 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/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.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/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.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/keyshare/src/threshold_keyshare.rscrates/net/src/document_publisher.rs
🧬 Code graph analysis (2)
crates/events/src/enclave_event/threshold_share_created.rs (1)
crates/trbfv/src/shares/bfv_encrypted.rs (1)
extract_for_party(189-193)
crates/keyshare/src/threshold_keyshare.rs (6)
crates/trbfv/src/helpers.rs (2)
deserialize_secret_key(41-44)serialize_secret_key(33-38)crates/keyshare/src/encryption_key_collector.rs (1)
setup(75-84)crates/events/src/enclave_event/mod.rs (1)
from_bytes(173-175)crates/utils/src/utility_types.rs (1)
from_bytes(25-27)crates/trbfv/src/shares/bfv_encrypted.rs (2)
len(196-198)encrypt_all(155-176)crates/events/src/enclave_event/threshold_share_created.rs (1)
num_parties(55-57)
⏰ 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: integration_prebuild
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: build_crisp_sdk
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
- GitHub Check: rust_integration
🔇 Additional comments (11)
crates/tests/tests/integration.rs (1)
290-315: LGTM! Test flow correctly implements BFV key exchange sequence.The test now properly waits for the two-phase protocol:
- BFV key collection: 5 EncryptionKeyCreated events (one per party)
- Share distribution: 25 ThresholdShareCreated events (5 parties × 5 targets)
This aligns with the per-recipient BFV encryption flow described in the PR objectives.
crates/events/src/enclave_event/threshold_share_created.rs (2)
37-58: LGTM! Per-party extraction logic is correct.The
extract_for_partymethod correctly:
- Extracts the recipient's share from both
sk_sssandesi_sss- Propagates
Noneif any extraction fails (usingcollect::<Option<Vec<_>>>)- Preserves
party_idandpk_sharefrom the original shareThe
usizeparameter type is consistent withBfvEncryptedShares::extract_for_partyAPI (see crates/trbfv/src/shares/bfv_encrypted.rs:188).
65-65: LGTM! Field addition enables per-party filtering.The
target_party_idfield correctly identifies the intended recipient of each share, enabling the per-party document filtering described in the PR objectives.crates/net/src/document_publisher.rs (4)
416-433: LGTM! Helper method correctly implements per-party filtering.The
publish_filteredmethod appropriately:
- Serializes the receivable document
- Creates metadata with
Filter::Item(party_id)for per-party routing- Publishes the filtered document request
This aligns with the per-recipient filtering objective described in the PR.
437-455: LGTM! Threshold share publishing correctly implements party filtering.The method properly:
- Skips external (already received) shares to prevent loops
- Publishes using
target_party_idfor per-recipient filtering- Logs the publishing action at info level
This implements the domain-level per-party splitting described in the PR objectives.
456-466: LGTM! Encryption key publishing correctly broadcasts to all parties.The method appropriately:
- Skips external events to prevent loops
- Uses an empty filter
vec![]to broadcast BFV public keys to all partiesBroadcasting is correct here because each party needs all other parties' BFV public keys to encrypt shares for them.
470-498: LGTM! Document reception correctly marks events as external.The method properly:
- Deserializes received documents
- Sets
external: truewhen publishing to the local bus- Handles both
ThresholdShareCreatedandEncryptionKeyCreatedvariantsThe
external: trueflag prevents re-publishing loops (as noted in past review comments, addressed in commit 47ba25f).crates/keyshare/src/threshold_keyshare.rs (4)
407-446: LGTM! BFV key generation follows secure practices.The method correctly:
- Uses
OsRngfor cryptographically secure key generation- Encrypts the BFV secret key with
SensitiveBytesbefore persisting- Stores the public key as
ArcBytes(not sensitive)- Transitions to
CollectingEncryptionKeysstate- Publishes
EncryptionKeyCreatedwithexternal: false(locally generated)Based on learnings, encrypted secrets are appropriately handled in this codebase.
448-478: LGTM! Encryption key collection transition is correct.The method properly:
- Extracts
CollectingEncryptionKeysDatafrom current state- Transitions to
GeneratingThresholdSharewith BFV keys and collected encryption keys- Preserves
sk_bfv,pk_bfv, and storescollected_encryption_keys- Triggers both share generation tasks (
GenEsiSssandGenPkShareAndSkSss)Past reviews confirmed that
collected_encryption_keysare already sorted by party_id in the collector.
634-714: LGTM! BFV encryption and per-party publishing is correct.The method properly:
- Deserializes collected BFV public keys (already sorted by party_id per past reviews)
- Decrypts local shares from encrypted storage
- Encrypts shares for all recipients using
BfvEncryptedShares::encrypt_allwithOsRng- Implements domain-level splitting by publishing one
ThresholdShareCreatedevent per recipient- Each event contains only the share data for that specific party (
extract_for_party)This correctly implements the per-recipient BFV encryption flow described in the PR objectives.
737-768: LGTM! BFV decryption logic correctly handles local and network shares.The method properly:
- Retrieves and deserializes the BFV secret key from encrypted storage
- Decrypts received shares using BFV with correct parameters
- Uses conditional indexing:
if len == 1 { 0 } else { party_id }
- Local shares (from self) contain all parties' data → use
party_idindex- Network shares (received via DHT) are pre-filtered → use index
0Past reviews confirmed this approach is correct with party-filtered distribution.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/keyshare/src/encryption_key_collector.rs (1)
18-50: LGTM - Timeout mechanism now implemented!This addresses the previous review feedback about missing timeout protection. The
AllEncryptionKeysCollectedcorrectly sorts keys byparty_idfor deterministic ordering, which is essential for correct BFV encryption/decryption indexing.
🧹 Nitpick comments (1)
crates/events/src/enclave_event/mod.rs (1)
303-306: Consider addingget_e3_idhandling for failure events.
EncryptionKeyCollectionFailedandThresholdShareCollectionFailedboth havee3_idfields but are not handled inget_e3_id(), causing it to returnNonefor these events. If E3-scoped routing or filtering is needed for failure events, consider adding them:🔎 Proposed diff
EnclaveEventData::TicketSubmitted(ref data) => Some(data.e3_id.clone()), EnclaveEventData::EncryptionKeyCreated(ref data) => Some(data.e3_id.clone()), + EnclaveEventData::EncryptionKeyCollectionFailed(ref data) => Some(data.e3_id.clone()), + EnclaveEventData::ThresholdShareCollectionFailed(ref data) => Some(data.e3_id.clone()), _ => None,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/events/src/enclave_event/encryption_key_collection_failed.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/threshold_share_collection_failed.rscrates/keyshare/src/encryption_key_collector.rscrates/keyshare/src/lib.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
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.
📚 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/encryption_key_collection_failed.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/threshold_share_collection_failed.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/events/src/enclave_event/encryption_key_collection_failed.rscrates/events/src/enclave_event/mod.rscrates/keyshare/src/lib.rscrates/keyshare/src/threshold_keyshare.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/events/src/enclave_event/mod.rscrates/keyshare/src/lib.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/events/src/enclave_event/mod.rscrates/keyshare/src/lib.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/events/src/enclave_event/mod.rscrates/keyshare/src/lib.rscrates/keyshare/src/threshold_keyshare.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/mod.rscrates/events/src/enclave_event/threshold_share_collection_failed.rscrates/keyshare/src/threshold_keyshare.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/keyshare/src/encryption_key_collector.rs
📚 Learning: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/keyshare/src/lib.rscrates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/lib.rscrates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.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-12T21:12:33.453Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 376
File: packages/ciphernode/sortition/src/sortition.rs:155-164
Timestamp: 2025-05-12T21:12:33.453Z
Learning: When refactoring Rust code to work with collections like HashMaps and moving values into closures, the compiler can sometimes determine that a value won't be used again and allow moving fields out of a borrowed value without requiring an explicit `move` keyword.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/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/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (3)
crates/events/src/enclave_event/threshold_share_collection_failed.rs (2)
crates/events/src/enclave_event/encryption_key_collection_failed.rs (1)
fmt(21-23)crates/events/src/enclave_event/mod.rs (1)
fmt(360-362)
crates/keyshare/src/threshold_share_collector.rs (1)
crates/keyshare/src/encryption_key_collector.rs (4)
setup(71-81)started(87-96)handle(101-152)handle(157-189)
crates/keyshare/src/encryption_key_collector.rs (2)
crates/keyshare/src/threshold_keyshare.rs (10)
from(69-73)new(190-208)new(320-330)handle(942-954)handle(959-964)handle(969-974)handle(979-984)handle(989-994)handle(999-1004)handle(1009-1014)crates/keyshare/src/threshold_share_collector.rs (2)
setup(43-53)started(59-68)
⏰ 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_e3_support_dev
- GitHub Check: test_net
- GitHub Check: build_crisp_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: build_ciphernode_image
🔇 Additional comments (17)
crates/events/src/enclave_event/threshold_share_collection_failed.rs (1)
1-24: LGTM!The
ThresholdShareCollectionFailedmessage struct is well-defined and follows the same patterns asEncryptionKeyCollectionFailedin this PR. TheDisplayimplementation using Debug formatting is consistent with other event types in the codebase (per the relevant code snippets frommod.rs).crates/keyshare/src/lib.rs (1)
7-13: LGTM!The new
encryption_key_collectormodule is properly added and only the necessary types (AllEncryptionKeysCollected,EncryptionKeyCollector) are re-exported, following the existing patterns in this crate.crates/events/src/enclave_event/encryption_key_collection_failed.rs (1)
1-24: LGTM!The
EncryptionKeyCollectionFailedmessage is well-structured and consistent withThresholdShareCollectionFailed. Both share the same pattern for failure events withe3_id,reason, andmissing_partiesfields.crates/events/src/enclave_event/mod.rs (2)
121-123: LGTM!The new
EnclaveEventDatavariants are correctly added, maintaining consistency with the existing enum structure.
335-339: LGTM!The
impl_into_event_data!macro invocation correctly includes all new event types, enabling automaticFromimplementations.crates/keyshare/src/threshold_share_collector.rs (4)
20-31: LGTM!The timeout mechanism is well-structured with a clear 120-second default, a dedicated
ThresholdShareCollectionTimeoutmessage, and theTimedOutstate variant. This follows the same pattern asEncryptionKeyCollector(60s timeout), providing consistent timeout handling across collectors.
43-68: LGTM!The
setup()method correctly initializes withe3_idfor traceability, and thestarted()hook properly schedules the timeout and stores the handle for later cancellation.
73-118: LGTM!The
ThresholdShareCreatedhandler correctly:
- Guards against processing in non-Collecting states
- Validates party_id against expected set
- Cancels timeout upon completion
- Sends
AllThresholdSharesCollectedto parent
121-156: LGTM!The timeout handler correctly guards against duplicate handling, logs missing parties with structured fields, transitions to
TimedOut, notifies the parent withThresholdShareCollectionFailed, and stops the actor. This addresses the previous review feedback about adding timeout mechanisms.crates/keyshare/src/encryption_key_collector.rs (3)
70-96: LGTM!The
setup()andstarted()implementations follow the same pattern asThresholdShareCollector, ensuring consistent behavior across collectors.
99-152: LGTM!The
EncryptionKeyCreatedhandler correctly:
- Guards against non-Collecting states
- Validates party_id against expected set
- Cancels timeout upon completion
- Converts to sorted
AllEncryptionKeysCollectedand sends to parent
155-189: LGTM!The timeout handler is consistent with
ThresholdShareCollector, properly logging, transitioning state, notifying parent, and stopping the actor.crates/keyshare/src/threshold_keyshare.rs (5)
76-100: LGTM!The new
CollectingEncryptionKeysDatastate and extensions toGeneratingThresholdShareDataandAggregatingDecryptionKeyproperly carry the BFV key material (sk_bfv,pk_bfv,collected_encryption_keys) through the state machine.
412-451: LGTM!The BFV key generation is secure:
- Uses
OsRngfor cryptographically secure randomness- Serializes and encrypts the secret key (
sk_bfv) with the cipher before storage- Properly transitions state and publishes the encryption key event
658-717: LGTM!The BFV-based share encryption flow is well-implemented:
- Collected keys are already sorted by
party_id(fromAllEncryptionKeysCollected)- Shares are encrypted for all recipients using BFV
- Per-party extraction (
extract_for_party) ensures each party only receives their sharetarget_party_idenables correct routing/filtering
744-773: LGTM - Dynamic index selection correctly handles both local and network shares.The logic at lines 748 and 765 correctly differentiates:
- Network shares (pre-extracted,
len == 1): use index 0- Local shares (full,
len > 1): useparty_idindexThis addresses the previous review concern about
clone_share(0)being incorrect for all cases.
1017-1068: LGTM!Both failure handlers properly:
- Log structured warnings with
e3_idandmissing_parties- Clear collector references
- Publish failure events to the bus for sync tracking (as noted by ryardley in past comments)
- Stop the actor to prevent proceeding in an inconsistent state
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/keyshare/src/threshold_keyshare.rs (1)
744-756: Clarify comment about share lengths.The comment states "Local share (from self) has all parties' shares, network shares are pre-extracted," but lines 705-718 show all shares (including local) go through
extract_for_partybefore publishing. This means all received shares should havelen() == 1in practice. The conditional at line 749 (and 766) appears to be defensive programming rather than handling an expected case.Consider updating the comment to clarify when
len() != 1would actually occur, or note that the conditional is defensive code to handle unexpected cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/keyshare/src/threshold_keyshare.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
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.
📚 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/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/threshold_keyshare.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: 2024-10-23T02:35:07.689Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/keyshare.rs:58-65
Timestamp: 2024-10-23T02:35:07.689Z
Learning: In `packages/ciphernode/keyshare/src/keyshare.rs`, the data being decrypted in the `get_secret` method of the `Keyshare` struct is not sensitive, so wrapping it with `Zeroizing` is unnecessary.
Applied to files:
crates/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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-05-12T21:12:33.453Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 376
File: packages/ciphernode/sortition/src/sortition.rs:155-164
Timestamp: 2025-05-12T21:12:33.453Z
Learning: When refactoring Rust code to work with collections like HashMaps and moving values into closures, the compiler can sometimes determine that a value won't be used again and allow moving fields out of a borrowed value without requiring an explicit `move` keyword.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
📚 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/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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T01:50:45.185Z
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.
Applied to files:
crates/keyshare/src/threshold_keyshare.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
crates/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.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/keyshare/src/threshold_keyshare.rs
🧬 Code graph analysis (1)
crates/keyshare/src/threshold_keyshare.rs (4)
crates/trbfv/src/helpers.rs (2)
deserialize_secret_key(41-44)serialize_secret_key(33-38)crates/keyshare/src/threshold_share_collector.rs (3)
setup(43-53)handle(73-118)handle(123-155)crates/keyshare/src/encryption_key_collector.rs (3)
setup(71-81)handle(101-152)handle(157-189)crates/trbfv/src/shares/bfv_encrypted.rs (2)
len(196-198)encrypt_all(155-176)
⏰ 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: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: test_net
🔇 Additional comments (2)
crates/keyshare/src/threshold_keyshare.rs (2)
1072-1087: LGTM—E3RequestComplete handler implemented correctly.The handler properly clears both collector references and triggers actor shutdown via the
Diemessage. This addresses the cleanup concern raised in previous review comments and follows the expected pattern for request completion.
413-452: LGTM—BFV encryption integration is well-implemented.The BFV key generation, share encryption, and decryption flow demonstrates solid cryptographic practices:
- OsRng used for key generation
- Secret keys protected with SensitiveBytes
- Per-party filtering ensures shares are only processed by intended recipients
- State transitions maintain consistency
- Comprehensive error handling with failure events
The integration successfully implements the per-recipient share encryption objectives from the PR.
Also applies to: 640-721, 726-788
|
amazing work @hmzakhalid @ryardley |
3 participants
closes #783
Overview
The flow consists of 5 phases:
State Machine
Sequence Diagram
sequenceDiagram autonumber participant EVM as Blockchain/EVM participant Bus as Event Bus participant TK as ThresholdKeyshare participant EKC as EncryptionKeyCollector participant TSC as ThresholdShareCollector participant MT as Multithread (Compute) participant Net as Network/DHT Note over TK: State: Init %% Phase 1: BFV Key Generation & Exchange rect rgb(230, 245, 255) Note right of EVM: Phase 1: BFV Key Exchange EVM->>Bus: CiphernodeSelected Bus->>TK: CiphernodeSelected TK->>TK: Generate BFV keypair (sk_bfv, pk_bfv) Note over TK: State: CollectingEncryptionKeys TK->>Bus: EncryptionKeyCreated (party_id, pk_bfv) Bus->>Net: Publish to DHT (broadcast) Bus->>EKC: EncryptionKeyCreated (local) Note over Net: All N parties publish their BFV public keys Net-->>Bus: EncryptionKeyCreated (remote parties) Bus->>EKC: EncryptionKeyCreated (party 0..N-1) EKC->>EKC: Collect all N keys EKC->>TK: AllEncryptionKeysCollected (sorted by party_id) end %% Phase 2: TrBFV Share Generation rect rgb(255, 245, 230) Note right of TK: Phase 2: Share Generation Note over TK: State: GeneratingThresholdShare par Parallel computation TK->>MT: GenEsiSss MT-->>TK: GenEsiSssResponse (esi_sss) and TK->>MT: GenPkShareAndSkSss MT-->>TK: GenPkShareAndSkSssResponse (pk_share, sk_sss) end Note over TK: State: AggregatingDecryptionKey TK->>TK: SharesGenerated: Encrypt shares with recipients' BFV public keys end %% Phase 3: Share Distribution (Domain-Level Splitting) rect rgb(245, 255, 230) Note right of TK: Phase 3: Share Distribution (N×N messages total) loop For each recipient party i (0..N) TK->>Bus: ThresholdShareCreated (target_party_id=i) Bus->>Net: Publish with Filter(party_id=i) end Note over Net: Each party receives only shares targeted to them Net-->>Bus: ThresholdShareCreated (filtered for this party) Bus->>TK: ThresholdShareCreated TK->>TK: Check: target_party_id == my_party_id? TK->>TSC: Forward if match TSC->>TSC: Collect N shares (one from each sender) TSC->>TK: AllThresholdSharesCollected end %% Phase 4: Decryption Key Aggregation rect rgb(255, 230, 245) Note right of TK: Phase 4: Key Aggregation TK->>TK: Decrypt received shares using own sk_bfv TK->>MT: CalculateDecryptionKey (sk_sss, esi_sss) MT-->>TK: CalculateDecryptionKeyResponse (sk_poly_sum, es_poly_sum) Note over TK: State: ReadyForDecryption TK->>Bus: KeyshareCreated (pk_share) end %% Phase 5: Decryption rect rgb(230, 230, 255) Note right of EVM: Phase 5: Decryption EVM->>Bus: CiphertextOutputPublished Bus->>TK: CiphertextOutputPublished Note over TK: State: Decrypting TK->>MT: CalculateDecryptionShare MT-->>TK: CalculateDecryptionShareResponse (d_share_poly) TK->>Bus: DecryptionshareCreated Note over TK: State: Completed endSummary by CodeRabbit
New Features
Public API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.