Adding from_hex and implementing FromStr for Hash

[phayes]

This PR adds:

1. from_hex for Hash as the inverse of to_hex, for completeness and convenience.
2. FromStr for Hash
3. Adds tests for both to_hex and from_hex

[oconnor663]

Thanks for this PR, and especially for including tests. A couple thoughts:

When I've run into this sort of thing, like in test vectors, I've usually found it easier to go the other way, to convert bytes into hex rather than hex into bytes. The main difference is that you never need to deal with error cases. The downside is that string comparisons aren't constant time, but I haven't found myself needing to do this in anything resembling production code. If going from hex back to Hash isn't common, there might not be enough usage to justify maintaining this feature. What do you think? (Note that users can always do this themselves, by parsing hex with e.g. the hex crate, and then doing a conversion from bytes.)

If we do want to support this, I imagine it would be more convenient for users to accept a &str, rather than an ArrayString. Forcing users to do the ArrayString conversion themselves make the feature a lot less convenient, especially since it'll usually entail adding a direct dependency on arrayvec.

[phayes]

@oconnor663, I totally agree about using &str - this is now fixed.

[oconnor663]

I'm leaning against including this, unless there's a significant use case.

[phayes]

Hi @oconnor663,

I would say that including this vs not including this depends on how much you want to recommend hexadecimal as the preferred encoding format for hash digests.

As for a use-case, this is what is looks like securely comparing two hashes that I have received from elsewhere with and without this PR.

Without the PR

use hex;
use blake3::Hash;

let hash1 = "7e9e4070a44c39a1b7f836e74383759aadcd341da9a3a71c8ecc3d2630c4f8d1";
let hash2 = "f81f4ad9582dac20141936d2d3930f21a57afd5a04cfed77cf4488e7da6e1196";

let hash1_bytes = hex::decode(hash1)?;
let hash2_bytes = hex::decode(hash2)?;

let hash1_array: [u8; 32];
let hash2_array: [u8; 32];

hash1_array.copy_from_slice(&hash1_bytes[..32]); // might panic
hash2_array.copy_from_slice(&hash2_bytes[..32]);; // might panic

let digest_1: Hash = hash1_array.into();
let digest_2: Hash = hash2_array.into();

if (hash1_digest != hash2_digest) {
    println!("Digests don't match")
}

With the PR

use blake3::Hash;

let hash1 = "7e9e4070a44c39a1b7f836e74383759aadcd341da9a3a71c8ecc3d2630c4f8d1";
let hash2 = "f81f4ad9582dac20141936d2d3930f21a57afd5a04cfed77cf4488e7da6e1196";

let digest_1: Hash = hash1.parse()?;
let digest_2: Hash = hash2.parse()?;

if (digest_1 != digest_2) {
    println!("Digests don't match")
}

Again, this comes back to how much you want to recommend hex as the serialization format for Hash and how much support you want to give for that recommendation.

[oconnor663]

I think if the user is conscientious enough to think about casting into blake3::Hash for constant time equality, they could also just as well do this:

use constant_time_eq::constant_time_eq;

let hash1 = "7e9e4070a44c39a1b7f836e74383759aadcd341da9a3a71c8ecc3d2630c4f8d1";
let hash2 = "f81f4ad9582dac20141936d2d3930f21a57afd5a04cfed77cf4488e7da6e1196";

if !constant_time_eq(hash1.as_bytes(), hash2.as_bytes()) {
    println!("Digests don't match")
}

That approach avoids dealing with parse errors, and it probably performs better to boot.

I'm going to close this without merging for now. Parsing hex feels much heavier-weight to me than emitting hex (for example the introduction of the ParseError enum), and I'd rather not go there unless there's a strong need.

[zaynetro]

My usecase involves serializing hashes and later reading them from disk for comparison. I pretty much prefer parsing them into Hash as it gives a clear picture what that field contains.

I agree that this library doesn't need to support reading hashes from hex. Although, it would be really nice to include some kind of documentation on how to do it.

Here is an example that works for me:

use std::convert::TryInto;
// hex is an external crate

let checksum = "993d1fe78155e0a92bb2e131946eca095bec8196dfd9d81b4b7d919b39b114a6";
let checksum_bytes = hex::decode(checksum).expect("decode hex");
let bytes: [u8; 32] = (&checksum_bytes[..32]).try_into().expect("slice to array");
let hash: Hash = bytes.into();

What do you think?

[oconnor663]

Good idea. I just added an example to the docs in that commit above.

[oconnor663]

I've been thinking more about this over time, and my feelings have changed a bit. (How embarrassing! 😄) I think if we do do this, it might be better to return Option rather than trying to provide a meaningful error type. Anyway, if I end up checking something in, I'll make sure to base it on phayes' commits to give credit where credit is due.

[oconnor663]

I've pushed the above merge commit and added some follow-on changes.