-
Notifications
You must be signed in to change notification settings - Fork 2
chore: added deterministic tests #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
==========================================
+ Coverage 88.07% 88.33% +0.25%
==========================================
Files 48 48
Lines 9201 9488 +287
Branches 9201 9488 +287
==========================================
+ Hits 8104 8381 +277
- Misses 652 662 +10
Partials 445 445 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DSharifi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for these changes! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to address this ugliness (and lack of efficiency, as afaik we send this over the wire) here. The bad part is that this would be a breaking change :| (incompatible with nodes running other version type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this exist?
I am unsure whether I really enjoy it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the file was proposed by @netrome as a better way to make snapshot tests on determinism.
My comment here refers to the serialization of the CKD types, which is defined by the underlying lib
d36be11 to
aac3704
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes non-deterministic RNG usage from the codebase by replacing OsRng with a seeded MockCryptoRng in tests. It adds deterministic test coverage using snapshot testing (via the insta crate) to verify that key cryptographic operations produce consistent outputs. The changes also update the test_utils API to accept RNG parameters instead of using OsRng internally, which is a minor breaking change to the test utilities API.
Key changes:
- Modified
MockCryptoRngto implementSeedableRngtrait for better RNG control - Updated test utility functions (
run_keygen,run_refresh,run_reshare, etc.) to accept RNG parameters - Added deterministic snapshot tests for keygen, refresh, and reshare operations across multiple signature schemes
- Added
instadependency for snapshot testing
Reviewed Changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/test_utils.rs | Added SeedableRng implementation to MockCryptoRng and updated helper functions to accept RNG parameters |
| src/dkg.rs | Refactored test functions to return results and accept RNG parameters for deterministic testing |
| src/eddsa/test.rs | Added deterministic tests with snapshot assertions and updated existing tests to use seeded RNG |
| src/ecdsa/mod.rs | Added deterministic snapshot tests for ECDSA keygen/refresh/reshare operations |
| src/ecdsa/robust_ecdsa/*.rs | Updated all tests to use deterministic RNG instead of OsRng |
| src/ecdsa/ot_based_ecdsa/*.rs | Updated OT-based ECDSA tests to use seeded RNG for determinism |
| src/confidential_key_derivation/*.rs | Added deterministic tests and updated existing tests to use seeded RNG |
| src/crypto/*.rs | Updated cryptographic proof tests to use MockCryptoRng instead of OsRng |
| benches/*.rs | Updated benchmarks to use MockCryptoRng for consistency |
| Cargo.toml/Cargo.lock | Added insta dependency for snapshot testing |
| snapshots/*.snap | Added snapshot files capturing deterministic outputs of key operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I did a brief review (loved your use of 42) . but I don't feel I have any useful feedback here. |
I think is fine to approve even without feedback, as this was a second review after quite a few review changes yesterday. Anyway it will change a little bit after another PR is merged and I rebase this one, so I will ask for the rubber stamp then. |
aac3704 to
91e267c
Compare
f30481e to
2f5fae7
Compare
chore: added support for refresh and reshare chore: added eddsa and ckd support feat: add support for ecdsa feat: make all tests deterministic fix: address nits feat: do determinism tests independently, use cargo-insta fix: minor after rebase fix: rebase fixes fix: minor
2f5fae7 to
8bee803
Compare
SimonRastikian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good overall,
Couple of comments about rng traits. Also I am not a big fan of the added files...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this exist?
I am unsure whether I really enjoy it.
12becff
12becff to
8bee803
Compare
Closes #107
OsRngin these files is preferred, as currently they are the closest we have to usage examples.