-
Notifications
You must be signed in to change notification settings - Fork 52
Implementation of Schnorr signature module for mithril-stm. #2761
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
base: main
Are you sure you want to change the base?
Conversation
b254ad2 to
1e07c07
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.
cargo-doc found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
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.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
curiecrypt
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.
First batch of reviews. It looks good at first glance.
00276b3 to
3ddf71f
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.
You should use full names even in function parameters. For example verification_key instead of vk.
For simplicity and descriptiveness, we can use random_scalar_1 instead of random_value_1. Consequently, we can call the generator * random_scalar_1 as random_point_1.
This will apply for all cases where we obtain a curve point by multiplying the generator with a scalar: For example: point_signature = generator * signature.
This kind of naming seems more descriptive and pleasant.
Final remark, if you use an underscore for separating numbers in one place, then you should do the same for all other similar cases. It should apply for separating x and y as well.
40c68d6 to
3b3a158
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 implements a Schnorr signature module for mithril-stm that will be used in the SNARK version of Mithril. The implementation provides signing and verification functionality using the Jubjub elliptic curve and Poseidon hash function, feature-gated behind future_snark.
Key Changes:
- Implementation of Schnorr signature scheme with signing keys, verification keys, and signature structures
- Utility functions for coordinate extraction and field conversions
- Comprehensive test coverage for signing, verification, and serialization
- New error type
SchnorrSignatureErrorfor Schnorr-specific errors
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| mithril-stm/src/schnorr_signature/verification_key.rs | Defines verification key type with serialization and conversion from signing key |
| mithril-stm/src/schnorr_signature/signing_key.rs | Implements signing key generation and Schnorr signature creation |
| mithril-stm/src/schnorr_signature/signature.rs | Defines signature structure and verification logic |
| mithril-stm/src/schnorr_signature/utils.rs | Utility functions for curve operations and field conversions |
| mithril-stm/src/schnorr_signature/mod.rs | Module organization with re-exports and integration tests |
| mithril-stm/src/error.rs | Adds SchnorrSignatureError enum with specialized error variants |
| mithril-stm/src/lib.rs | Exposes Schnorr types in public API behind future_snark feature |
| mithril-stm/Cargo.toml | Adds dependencies for Jubjub curve and Poseidon hash (dusk-jubjub, dusk-poseidon, ff, group) |
| mithril-stm/benches/schnorr_sig.rs | Benchmark for individual Schnorr signature verification |
| Cargo.lock | Dependency resolution including new cryptographic libraries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
c8c54f3 to
c65c691
Compare
Content
This PR includes a new module for
mithril-stmcontaining an implementation for the Schnorr signature that will be used in the SNARK version of mithril.Pre-submit checklist
Benchmark results
The preliminary benchmark results (on my machine) of the two signature schemes are the following.
For individual signature:
For individual verification:
The benchmark are using the Dusk library for running Poseidon. Dusk Poseidon hash:
Comments
For from_bytes() functions, do we want to fail/warn if we receive more bytes than needed?
In the sign function of SchnorrSigningKey, how do we want to handle the randomness?
Add loops when generating random values to avoid 0? Done for the signing key for now.
Removed function hash_msg_to_jubjub for now, we might need it somewhere outside the schnorr signature module.
Issue(s)
Relates to #2756