Implement Try and Increment when converting hash to bigint #128
Conversation
| @@ -38,8 +42,22 @@ impl Blake { | |||
| } | |||
|
|
|||
| pub fn result_scalar<E: Curve>(&self) -> Scalar<E> { | |||
| let n = self.result_bigint(); | |||
| Scalar::from_bigint(&n) | |||
| let scalar_len = E::Scalar::SCALAR_LENGTH; | |||
There was a problem hiding this comment.
it sucks that we need this code duplication,
I think we should remove those implementations and make the code that requires hashes generic over Digest and that way the user can choose their own hash function (and RustCrypto already has a blake2b implementation with simd acceleration: https://github.com/RustCrypto/hashes/tree/master/blake2/src)
| ); | ||
| // Try and increment. | ||
| for i in 0u32.. { | ||
| let starting_state = self.clone(); |
There was a problem hiding this comment.
I noticed that we have to add + Clone in a lot of places because of this .clone(). It's not really critical, but we can eliminate this by finding first H(data || 0x1 || 0x2 || ... || i) instead of H(data || i). Are there any downsides of this approach?
There was a problem hiding this comment.
good question, I'll try to think if this can introduce any attacks.
(small note, AFAIK all hash functions implement Clone)
There was a problem hiding this comment.
Yeah, all hashes must implement Clone. My suggestion is only about saving few keystrokes. If you think it might introduce any attacks, let's leave it as is
There was a problem hiding this comment.
Btw do we even need to specify + Clone constraint? According to the documentation, Digest trait is implemented for any D that D: Clone + FixedOutput + .... Can't Rust deduce that any Digest implements Clone? See https://docs.rs/digest/0.9.0/digest/trait.Digest.html#impl-Digest
There was a problem hiding this comment.
It's implemented for it doesn't inherit it :/
I can instead do D: FixedOutput + Update + Clone but sadly I can't remove clone
survived
force-pushed
the
try-and-increment
branch
from
32b0263
to
bb157de
Compare
survived
force-pushed
the
try-and-increment
branch
from
bb157de
to
64b2af3
Compare
survived
force-pushed
the
try-and-increment
branch
from
11c8420
to
d973d16
Compare
|
@survived Your changes look great :) |
This algorithm should improve performance in the regular case (no conversion to big int and serializing that), and should also improve security in curves where 2^n-q > ε.
Note that now if you try to convert a small hash into a bigger scalar this will panic,
we could return an error instead but I think if this happens then it is a programmer error that requires fixing the code, you can't really "handle" an error like that. so I think a panic is the right thing.