Conversation
Consolidated Tests Results 2026-02-25 - 17:22:59Test ResultsDetails
test-reporter: Run #590
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
There was a problem hiding this comment.
Pull request overview
This PR streamlines the migration approach for the KMS server by consolidating migration logic to run immediately at startup when calling kms-server. The changes refactor how legacy FHE keys and PRSS (Pseudo-Random Secret Sharing) setups are handled during migration from version 0.12.x to 0.13.1.
Changes:
- Introduced
migrate_to_0_13_1()function that consolidates FHE key and PRSS migration at server startup - Refactored FHE key migration to preserve legacy data instead of immediately deleting it, with cleanup deferred to version 0.14.0
- Changed PRSS migration to convert legacy format to new combined format at boot and delete legacy data immediately
- Removed PRSS overwrite logic in epoch_manager to prevent silently hiding bugs
- Moved
init_all_prss_from_storage()call to after PRSS initialization in the startup sequence
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/service/src/bin/kms-server.rs | Updated to call new consolidated migration function at startup with threshold and num_parties parameters |
| core/service/src/engine/migration.rs | Added migrate_to_0_13_1() and migrate_to_0_14_0() functions; refactored FHE key migration to not delete legacy data immediately; moved PRSS migration logic from epoch_manager; added comprehensive tests |
| core/service/src/engine/threshold/service/epoch_manager.rs | Removed init_legacy_prss_from_storage() method and PRSS overwrite checking logic; updated documentation |
| core/service/src/engine/threshold/service/kms_impl.rs | Reordered initialization to call init_all_prss_from_storage() after conditional PRSS creation |
| core/service/src/engine/threshold/service/session.rs | Added #[allow(dead_code)] to num_parties() method which is no longer used after refactoring |
Comments suppressed due to low confidence (1)
core/service/src/engine/threshold/service/epoch_manager.rs:330
- The removal of PRSS overwrite checking combined with storage's non-overwriting behavior creates a potential inconsistency. If init_prss is called twice with the same epoch_id: (1) the first call generates and stores PRSS to disk, (2) the second call generates a different PRSS, attempts to store it (which silently succeeds without writing due to line 284-289 in file.rs), then adds this new PRSS to session_maker (line 330), resulting in in-memory PRSS differing from on-disk PRSS. The comment "Ensure data can be stored before updating the model in ram" (line 321) suggests the intent is to prevent this, but store_versioned_at_request_id returns Ok() even when it skips writing. Consider either: (1) checking if data already exists before running the expensive PRSS protocol, or (2) making store_versioned_at_request_id return an error when data already exists for this critical use case.
// Ensure data can be stored before updating the model in ram
store_versioned_at_request_id(
&mut (*priv_storage),
&(*epoch_id).into(),
&prss,
&PrivDataType::PrssSetupCombined.to_string(),
)
.await?;
session_maker.add_epoch(*epoch_id, prss).await;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
kc1212
left a comment
There was a problem hiding this comment.
Thanks for this, I think, in addition to all this, we need to check that our storage backend works properly with the two types FheKeyInfo storage patterns.
Could you add some tests, for all the storage backends, so that when doing things like find all epoch IDs for FheKeyInfo, it only returns the epoch IDs that matches this pattern
FheKeyInfo/<epoch_id>/<key_id>
and does not return the <key_id> as epoch ID in this pattern FheKeyInfo/<key_id>?
|
More concretely, the functions like |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kc1212
left a comment
There was a problem hiding this comment.
I just have one concern around the removal of the PRSS existance check. the rest are all ok
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description of changes
kms-server.Issue ticket number and link
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md