Generalise to reddsa crate
#87
Conversation
| // This should not exist, but is necessary to use zeroize::DefaultIsZeroes. | ||
| impl Default for SpendAuth { | ||
| fn default() -> Self { | ||
| unimplemented!() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is necessary because a DefaultIsZeroes: Default bound on Foo<S> requires S: Default. Per the comment in the commit message for 2745d37 we could replace DefaultIsZeroes with explicit Zeroize and Drop derives; I think we should do that (because having a panicking S::default() on an unconstructable type will create a Bad Time for someone.
There was a problem hiding this comment.
I created an issue to do this later ZcashFoundation/reddsa#1
|
I've rebased this on #131 so it includes both the latest The last time we discussed this PR, we decided it would make sense for it to implement a workspace with both |
str4d
force-pushed
the
reddsa
branch
2 times, most recently
from
dbf1a6c to
ca266d5
Compare
|
To unblock an |
The prior `SpendAuth` and `Binding` enums have been renamed to
`sapling::{SpendAuth, Binding}`. These might subsequently be removed
from the crate entirely (moving into a wrapping `redjubjub` crate).
The code assumes that scalar and point representations are [u8; 32],
which will be the case for all curves we instantiate RedDSA with for
Zcash.
There was a problem hiding this comment.
Looks good, thank you!
There are some minor readability issues which we can address later and I created issues for them, also for some TODOs you mentioned:
- Remove Default impls with unimplemented! reddsa#1
- Simplify creating types from bytes (remove
default()/copy_from_slice()/from_repr()) reddsa#2 - Remove Default impls with unimplemented! reddsa#1
The idea here is to not merge this PR but move it to the reddsa repo, right?
| // This should not exist, but is necessary to use zeroize::DefaultIsZeroes. | ||
| impl Default for SpendAuth { | ||
| fn default() -> Self { | ||
| unimplemented!() | ||
| } | ||
| } |
There was a problem hiding this comment.
I created an issue to do this later ZcashFoundation/reddsa#1
Correct. Eventually we will likely rectify the two repositories into a single workspace, but to unblock the This PR is now ZcashFoundation/reddsa@878dd13, and that repo also has two extra commits with Orchard support: ZcashFoundation/reddsa@878dd13...b915593 |
An implementation of RedJubjub or RedPallas on its own is very small, and something that could be implemented separately for
zebradandzcashd(although the Zcash Foundation has already proposed replacingzcashd's implementation of RedJubjub with this one).However, the introduction of FROST for threshold multisignatures adds significant implementation complexity. I worked out a rough breakdown of the codebase (module lines, including whitespace and comments):
FROST in this crate is building on RedDSA, which was intentionally designed to be generic and curve-independent; this means that it should equally apply to RedJubjub (for Sapling) and RedPallas (for Orchard). But reimplementing all this logic for a second time in Rust is not an effective use of engineering resources (that could instead be directed to this implementation).
This PR takes the generalisation approach:
redjubjubtoreddsa(a crate name I have already secured for this purpose).SpendAuthandBindingtraits are introduced. Both traits depend on the existing sealed traits (and use them to inherit common curve details).saplingmodule.S: SpendAuth.Still todo:
redjubjubcrate that wrapsreddsa. This would probably make sense to live in this repo as part of a workspace.reddsa. This might require un-sealing some traits, and then re-applying seals inredjubjub.