Better test coverage for MPC context lifecycle (PR 3/7) - #813
Draft
dvdplm wants to merge 18 commits into
Draft
Conversation
…recise points in the series of storage operations a test uses.
…ge. Tests that do not care just use normal RamStorage.
- Cleanup succeeds - Cleanup itself fails - The resharing fixture now includes: - Existing public key and CRS material. - Unrelated public material. - Private material belonging to the previous epoch. - Private material belonging to an unrelated epoch. - Durable epoch markers for both the previous and new epochs. - Assertions now compare complete before/after storage state and inspect mutation/fault journals.
Storage events as a seq of ops, add support to assert on either before after the op
… public and private store outcome and compare to the the pre-write state. When either store fails, purge only entries that the write could have created. Add test coverage for empty and mixed states for threshold key and CRS pairs, with failures before and after mutation.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves correctness and test coverage around MPC context lifecycle operations (create/destroy) and paired public/private CryptoMaterialStorage::write_all behavior, specifically to keep persistent storage and in-memory/cache/session state consistent and make failures retryable.
Changes:
- Introduces test support utilities (storage entry identifiers, event recording, and fault phases) plus a new fault-injecting
FailingRamStorageto precisely model before/after-mutation failures. - Refactors
CryptoMaterialStorage::write_allto track per-half write outcomes, avoid purging pre-existing entries on partial failures, and propagate richer store outcomes internally. - Updates context manager lifecycle flows to serialize existence-check + update via a mutex, reorder storage vs cache/session mutations for retryability, and add regression tests for failure modes (including confirmed deletes and backup failures).
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/threshold-execution/src/online/reshare.rs | Minor formatting adjustment in a reshare test helper (brace line changed). |
| core/service/src/vault/storage/test_support.rs | Adds shared test-only types for storage entry identification, fault phases, event/outcome recording, and event comparison. |
| core/service/src/vault/storage/ram/failing.rs | Adds a new test-only FailingRamStorage wrapper with entry-specific fault injection and event/state tracking. |
| core/service/src/vault/storage/ram.rs | Wires the new FailingRamStorage test module and removes the previous test-only failing storage implementation. |
| core/service/src/vault/storage/mod.rs | Exposes the new test_support module under cfg(test). |
| core/service/src/vault/storage/crypto_material/tests/storage_side_effects/support.rs | Adds fixtures/helpers for paired pub/priv write_all side-effect testing. |
| core/service/src/vault/storage/crypto_material/tests/storage_side_effects/cases.rs | Adds parameterized tests validating rollback and preservation semantics for paired writes and context-info writes. |
| core/service/src/vault/storage/crypto_material/tests/storage_side_effects.rs | Adds the test module entrypoint for write_all side-effect tests. |
| core/service/src/vault/storage/crypto_material/tests/migration.rs | Adjusts migration tests to the updated threshold storage setup helper signature. |
| core/service/src/vault/storage/crypto_material/tests.rs | Updates tests to new failure injection approach and to the updated write APIs/outcomes; adds storage_side_effects module. |
| core/service/src/vault/storage/crypto_material/mod.rs | Re-exports StorageError for broader in-crate use (e.g., context manager logic). |
| core/service/src/vault/storage/crypto_material/base.rs | Refactors write_all and related helpers to track store outcomes and purge only newly-created entries; returns StoreWriteOutcome from internal store helpers. |
| core/service/src/engine/threshold/service/epoch_manager/tests/failed_reshare.rs | Adds regression tests for failed reshare rollback semantics and retry state retention. |
| core/service/src/engine/threshold/service/epoch_manager.rs | Updates epoch-manager tests to new failing storage behavior and adds the failed-reshare test module. |
| core/service/src/engine/context_manager/tests/lifecycle_side_effects/support.rs | Adds fixtures for MPC context create/destroy lifecycle failure tests with storage/session/cache state assertions. |
| core/service/src/engine/context_manager/tests/lifecycle_side_effects/cases.rs | Adds lifecycle side-effect tests covering failures, retries, locking/serialization, backup failures, and session rollback. |
| core/service/src/engine/context_manager/tests/lifecycle_side_effects.rs | Adds the test module entrypoint for context lifecycle side-effect tests. |
| core/service/src/engine/context_manager.rs | Serializes lifecycle updates with a mutex; reorders storage vs cache/session mutations; adds “confirmed delete” handling and supporting helpers. |
| ai-docs/ARCHITECTURE.md | Documents paired write semantics and updated lifecycle/rollback expectations. |
Suppressed comments (1)
core/service/src/vault/storage/test_support.rs:18
- Minor grammar issue in the doc comment: "(is used for every epoch)" should be "(they are used for every epoch)".
/// Public halves have no epoch (is used for every epoch). Each private half has an epoch and contains the party's
/// material for that epoch.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+8
to
+10
| /// The containing storage supplies the `PUB` or `PRIV` root. The entry itself contains the | ||
| /// remaining path parts. The paired threshold writes covered by these tests use: | ||
| /// |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked upon #804, hence the draft status.
Test and fix MPC context write operations ordering across persistent storage and in-memory state.
Before this PR, a failed centralized context creation could leave the cache state out of sync with storage. A failed threshold context deletion removed the context from the session maker before storage deletion succeeded, which made the operation difficult to retry.
This PR:
Part of https://github.com/zama-ai/kms-internal/issues/3183.