Update to new Scalar API
#293
Conversation
|
Ready for final review. Made a small change. Done. |
|
@tarcieri ready again. Ended up not inlining |
|
Ok should be ready to go. This also happens to fix the rare behavior change mentioned in #299 where After this, I think this and |
I'm still definitely not sold on having a bunch of features for these backends. I feel like we managed to remove a bunch of the complexity and knobs that introduced unnecessary choices which can be made by intermediate dependencies, only to reintroduce them. We should probably reopen one of the issues we had discussing this or make a new one to (re-)discuss backend selection. There were many, many people opposed to the use of crate features for this and I think it provides unnecessary complexity with no immediate use cases. |
|
Ok agreed. When you get a chance can you approve this? |
|
Oh I'll change that |
|
Ok I believe all the unwraps are entirely gone now |
| // `scalar_bytes` and `scalar` are separate, because the public key is computed as an unreduced | ||
| // scalar multiplication (ie `mul_base_clamped`), whereas the signing operations are done | ||
| // modulo l. | ||
| pub(crate) scalar_bytes: [u8; 32], |
There was a problem hiding this comment.
I guess I overlooked this before.
@rozbb there are use cases like Ed25519-BIP32 where the private scalar is computed rather than clamped from bytes.
Adding this field means you can no longer construct an ExpandedSecretKey directly from a derived Scalar and prefix using the struct literal syntax ExpandedSecretKey { scalar, hash_prefix } and have to go through one of the constructor methods.
I guess the best you could do now if you have a scalar and a hash_prefix is to serialize the scalar with Scalar::to_bytes and concatenate it with the hash_prefix and use ExpandedSecretKey::from_bytes?
Also all that aside, I find a mixture of public and private fields weird. If you really want to go down this path it would probably be good to make scalar and hash_prefix into methods, removing the fields from pub visibility.
There was a problem hiding this comment.
Opened a PR to make the other fields private: dalek-cryptography/curve25519-dalek#544
Motivation can be found in this comment: dalek-cryptography/ed25519-dalek#293 (review) Replaces public fields with private fields + accessor methods. Also adds an `ExpandedSecretKey::from_scalar_and_prefix` constructor which makes it possible to construct `ExpandedSecretKey` using a `Scalar` again, which is useful for protocols that derive the scalar such as Ed25519-BIP32. Additionally defines a `HashPrefix` type alias to make it semantically clear which `[u8; 32]` is involved in type signatures.
Motivation can be found in this comment: dalek-cryptography/ed25519-dalek#293 (review) Replaces public fields with private fields + accessor methods. Also adds an `ExpandedSecretKey::from_scalar_and_prefix` constructor which makes it possible to construct `ExpandedSecretKey` using a `Scalar` again, which is useful for protocols that derive the scalar such as Ed25519-BIP32. Additionally defines a `HashPrefix` type alias to make it semantically clear which `[u8; 32]` is involved in type signatures.
This removes all uses of
Scalar::{from_bits, from_bits_clamped}. Some comments inline.curve25519-dalekdependency is pinned to Git inCargo.toml, with revisionf460ae1specified inCargo.lock. I'll move this back to a semver dep once a newcurve25519-dalekRC is cut.