I'm excited to share my first significant Rust project with the community. I would be happy to receive your feedback and reviews (for both the math implementation and the code part).
Project Overview
GitHub Repository: Schnorr Signature
Description: This library provides two algorithms two generate Schnorr signatures: MuSig and the classic one. MuSig implementation includes. You could find implementation details in the README.
My Interests
I'm a blockchain researcher with a keen interest in cryptography learning. This project aligns with my passion for exploring cryptographic techniques within the blockchain space.
Contributing
I welcome contributions, issues, and pull requests on the GitHub repository. If you'd like to get involved or collaborate on this project, please feel free to reach out.
Thank you for taking the time to review my project, and I look forward to your feedback and insights.
I don’t have time to review in details, and I don’t think I’m qualified enough to speak up about cryptography-related code. However I took a look out of curiosity, and here are a few remarks:
First, I have to mention that the code is really clean and easy to read. It’s also well documented. It’s a pleasure seeing that.
I see you didn’t follow the standard project layout for the examples. That is, there is an example folder, but instead of holding a top-level .rs file per example, there are other cargo packages with their own Cargo.toml. This is not necessarily bad, but deviates a bit. Putting the examples in the same package is also useful for some features such as example scrapping (currently unstable, but already enabled by docs.rs). You could check the repository of bevy to see how this is used in action. It’s also easier to discover and run the examples using cargo run --example=….
I feel this could be split in two crates. Or at least, tracing-subscriber, tracing-log and tracing-bunyan-formatter are crates that are typically used in application packages rather than libraries. It’s likely the telemetry module should be behind a feature flag or not be part of this crate at all.
Some problems/observations, not in order of importance:
Run Clippy (cargo clippy) on your code to receive automatic code quality feedback
For implementing Debug (e.g. on RandomNonce), use f.debug_struct(). Currently, your implementation doesn't handle any flags, especially the {:#?} specifier for verbose/indented printing.
Instead of having an inherent RandomNonce::new_rand() method, implement the Distribution trait for your type. This makes it possible to use it with any PRNG. Or at least make the method generic over an R: CryptoRng so that it can be used with any secure PRNG.
The above two suggestions also apply to KeyPair.
telemetry::init_subscriber() contains two expect()s. You should probably not make the decision to panic on behalf of the user. Return a Result instead.
SchnorrSignature::sign() takes a &String. That's an anti-pattern. Taking an owned &String by reference means that you can't pass anything except a String, i.e., you have to have an owned string in the first place. That means an extra allocation if you only have e.g. a &str. If you only need to read that parameter by reference, then take a &str or a generic bounded by AsRef<str> instead.
The same goes for verify() and calc_hash_rpm().
The same is true for the &Vec argument of musig_math::aggregate_public_keys(). You should take a &[RistrettoPoint] instead. And the same goes for the other functions in this model: calc_cap_l(), calc_hash_xrm(), aggregate_nonces(), partial_signature(), etc.
The same is true for musig_protocol::Aggregator::message
It is bad practice to simply leave off the elided lifetime parameters of UDTs. The signature of Aggregator::new() should contain the elided lifetime, like this:
But then it's weird that Aggregator<'a, _> returns Aggregator<'_, _>, i.e., an aggregator with a different lifetime. You should probably take a &'a str and return an Aggregator<'a, _> instead.
The same is true for all other constructors of this type.
Aggregator::aggregate_signatures() contains multiple unwrap()s and an explicit panic. You shouldn't do that. It's entirely expected that untrusted signatures are invalid, that's the whole point of the verification. You should again be returning Results instead. The same goes for get_aggregated_signature().
// Clear the concatenated_bytes vector to avoid unintentional data leakage
concatenated_bytes.iter_mut().for_each(|byte| *byte = 0);
The compiler sees that you write to the buffer without reading it, and will entirely remove this useless operation.
Securely wiping memory is a quite complex and subtle problem. Unless you're willing to take a deep dive into its complexities, you should just use a crate which deals with the issues. zeroize is the currently standard solution in the Rust ecosystem. The crate secrecy, which you are already using, also provides a higher-level solution.
impl Clone for RandomNonce is a security footgun. Nonces should not be reused. Cloning a nonce makes it easier to accidentally use it twice. It seems that you use the Clone impl to sidestep borrow checker issues. You should consider restructuring your code instead, and make RandomNonce move-only (i.e. don't implement either Clone or Copy, and leave a comment that you did it intentionally).
Signature creation & verification take &String. Besides the mentioned issue that it should be &str, since this is strictly more flexible and efficient API, it is semantically problematic. String is an UTF-8 text string. Signatures aren't generated for text, they are generated for byte sequences. This is both more general, since a byte sequence doesn't need to represent valid UTF-8 text, and more precise. UTF is complicated. In particular, visually the same text can be represented in many different ways. This means text must be properly normalized before signing. Using byte slices &[u8] makes it more explicit, since it requires some conversion from text to a byte slice (although one can still use the incorrect str::bytes conversion).
General notes:
You are using ordinary comments // comment for documentation of functions. Instead, you should use documentation comments/// doc. They allow automatic generation of documentation for your crate via rustdoc, including documentation on docs.rs, and interact better with other tooling, including IDEs.
You are using tracing and serde by default in your crate. If you plan to turn it into a general-purpose library, it may be a good idea to make those dependencies optional, since they can significantly increase compile times:
[dependencies]
curve25519-dalek = { version = "4.1.1", features = ["rand_core", "digest", "serde"] }
rand = "0.8.5"
sha2 = "0.10.8"
log = "0.4"
serde = { version = "1", features = ["derive"], optional = true }
secrecy = { version = "0.8", features = ["serde"] }
tracing = { version = "0.1.37", optional = true }
tracing-subscriber = { version = "0.3", features = ["registry", "env-filter"], optional = true }
tracing-bunyan-formatter = { version = "0.3.1", optional = true }
tracing-log = { version = "0.1.1", optional = true }
[features]
tracing = [ "dep:tracing", "dep:tracing-log", "dep:tracing-subscriber", "dep:tracing-bunyan-formatter"]
Note that here serde is an optional dependency, which implicitly creates a serde feature. On the other hand, tracing-related dependencies are listed with dep: prefix in the specification of the tracing feature, which means that they don't have associated implicit features. Instead, they are enabled or disabled all at once, via the explicitly declared tracing feature.
In your code, you would gate the relevant functionality behind #[cfg] or #[cfg_attr]. For example,
Of course, this increases the code complexity, so you may also keep the dependencies as required. For an internal crate of some project, this is a very reasonable solution.
You have a huge API surface, with almost all modules, functions and types in your crate being marked as pub. Generally that's a poor practice. It makes using your crate more complicated for the end users, since it's unclear which functions are important, and which are unlikely to ever be useful. For example, do you really expect end users to call aggregate_public_keys or calc_cap_l? Exposing private functions and types can also make your crates longer to compile, and less optimized (though in practice it may be insignificant).
The Aggregator struct has multiple optional fields. Only some fields are set for each specific step of the multisig protocol, and you use Option::unwrap to select the expected fields in the code. This is error-prone. One can easily unwrap the wrong fields, or forget to erase the fields from the previous steps. A better design would be to turn Aggregator into an enum, with different variants representing different steps of the protocol, or even create separate structs for the separate states. For details, see this famous post on the design of state machines in Rust.
State transitions (aggregate_signatures, calc_aggregated_signature etc) should also generally take the state by value rather than by reference. I.e. instead of