We closed a similar PR in redjubjub ZcashFoundation/redjubjub#101 since it was out of date, and now redjujub is just a wrapper for reddsa. But to be sure we can review the unwrap() usages in this crate.

Crate serde is imported as optional, but conditional compilation for module messages is missing. Thus

cargo build --no-default-features

fails.

To create certain types from bytes we need to go through a bunch of steps, e.g.

let mut repr = <S::Scalar as PrimeField>::Repr::default();
repr.as_mut().copy_from_slice(&s_bytes);
let maybe_scalar = S::Scalar::from_repr(repr);

It seems this could be improved by const_evaluatable_checked which has not stabilized yet. Check if that is the best solution or if there is an alternative, and implement it if possible.

Using DefaultIsZeroes requires us to implement Default on SpendAuth and Binding, but those are supposed to be unconstructable. This was solved by adding a Default impl which calls unimplemented! but that doesn't seem ideal.

Check for an alternative and remove the Default impls. One possibility is to explicitly implement Zeroize and Drop. See https://github.com/ZcashFoundation/redjubjub/pull/87/files#r628834530 and ZcashFoundation/redjubjub@2745d37 for context.

Add tests for the PositiveY code used by FROST to ensure the pubkey has a positive Y in the Pallas ciphersuite.

Also improve its docs to explain why it's needed (point to spec) and how it works.

If the developers who are familiar with this crate are away, we need instructions so we can do a release.

They could be something like:
https://github.com/ZcashFoundation/zcash_script#updating-this-crate

@saleemrashid commented on Tue Feb 25 2020

Current Plan

• Keep thiserror usage behind thiserror feature — thiserror does not support #![no_std] and hardware wallets will want to minimize code size so core::fmt is out of the question.

• Add trait Blake2b and provide implementation for blake2b_simd when feature is enabled. Hardware wallets have their own BLAKE2b implementations and will want to minimize code size by not adding another. For other embedded systems, there may be implementations more suitable than blake2b_simd. This requires adding a Blake2b type parameter to struct HStar, PublicKey::verify, and SecretKey::sign.

• Wrap blake2b_simd and thiserror in an std feature, and add #![cfg_attr(not(feature = "std"), no_std)]. Replace all use std:: with use core::.

Issues

• The BLAKE2b issue will be present in other crates that hardware wallets need to use, so trait Blake2b would be useful there. Therefore, it doesn't seem like a good idea to define that trait in this crate. Which crate should it go in?

• How much code size does rand_core add? This is important for hardware wallets, especially ones that are not already using Rust as they will also incur the code size cost of the standard library. If it adds too much code size, perhaps another signing method should be added to SecretKey, which accepts the entropy as a parameter, rather than accepting a CryptoRng instance.

We can use where to make some code more readable, mainly batch.rs

For example:

impl<S: SpendAuth, B: Binding<Scalar = S::Scalar, Point = S::Point>> Default for Verifier<S, B> {

could be

impl<S, B> Default for Verifier<S, B>
where
    S: SpendAuth,
    B: Binding<Scalar = S::Scalar, Point = S::Point>,
{

Improve this and any other similar instances, if any.

zebra had a duplicated redpallas code which was removed and now it's using this crate.

However, it still has a redpallas benchmark (zebra-chain/benches/redpallas.rs) that we should move here.

Delete it from zebra and add it here.

These are causing the coverage job which relies on nightly to go red

This should be

    /// The batch verification equation is:
    ///
    /// h_G * ( -[sum(z_i * s_i)]P_G + sum(\[z_i\]R_i) + sum([z_i * c_i]VK_i) ) = 0_G
    ///
    /// as given in https://zips.z.cash/protocol/protocol.pdf#reddsabatchvalidate
    /// (the terms are split out so that we can use multiscalar multiplication speedups).

Since the split-out form is already the one given in the spec, there is no need to derive it. More importantly, the cofactor multiplication applies to all terms, not just the term with base $\mathcal{P}_ {\mathbb{G}}$. Since * has higher precedence than +, the given version is wrong. The equation further on in the comment that splits out $\mathcal{P}_ {SpendAuth}$ and $\mathcal{P}_ {Binding}$ is correct.

This does not affect the implementation; that uses Self::Point::is_small_order() to do the cofactor multiplication on the overall sum, which is correct.

The comment in the corresponding redjubjub code is also wrong in the same way; see ZcashFoundation/redjubjub#163

#29 seemed to raise the MSRV.

We should fix that and test the MSRV in CI to make sure we don't break it accidentally again.