fix: Digest::try_from() returns NotCanonical error

[dan-da]

Addresses #195.

Any attempt to instantiate a Digest from bytes or a string with embedded non-canonical u64 values now results in an error.

Note that it is still possible to instantiate a Digest from BFieldElements that were created from non-canonical u64. Maybe that's ok...?

Changes:

• adds TryFromDigestError::NotCanonical
• adds BFieldElement::is_canonical()
• Digest::from_str() returns NotCanonical error
• Digest::try_from(bytes) returns NotCanonical error
• adds Digest test try_from_bytes_not_canonical()
• adds Digest test from_str_not_canonical()
• adds Canonical test in digest_from_invalid_hex_errors()

---

Commentary:

I am not entirely happy with this solution/pr for two reasons:

1. it only partially addresses the issue. It remains possible to bypass the canonical checks by instantiating Digest from BFieldElements directly. That feels wrong to me (leaky abstraction), but maybe it's actually Ok. opinions welcome.
2. It seems cleaner to perform the canonical check(s) inside BFieldElement rather than Digest methods.

regarding (2) I initially started with this approach, and impl'd TryFrom bytes for BFieldElement. This required getting rid of impl From bytes for BFieldElement. That was fine as it doesn't seem to be used. However, the TryFrom bytes is only a partial solution as BFieldElement still has a bunch of impl From for u64, u128, i32 etc as well as impl FromStr. I felt I was likely to cause downstream breakage changing all these to TryFrom and also it was becoming a bigger project than anticipated, so I backed off. I still feel that is the most correct fix if we really want to enforce the canonical check. I pushed an alternative branch with this approach.

However, for basic bytes/string serialization, the present PR suffices, is smaller, and I verified that downstream neptune crates have no build errors. I believe this is also closer to what was discussed/proposed in #125.

Another consideration is that its possible we might be instantiating Digest(s) from non-canonical BFieldElement inputs somewhere in the neptune stack. So adding a check inside BFieldElement would have potential to break some logic (not just syntax). I'm unsure how likely this is or not.

At this point we could merge this PR as-is, or pursue the other branch/approach. I don't have a strong preference and am open to suggestions/feedback.

a final note: Digest also has a TryFrom BigUint. I looked at it, but it wasn't immediately clear to me if a canonical check is needed or exactly how to do it, so I didn't touch it. That might be another leak.

[coveralls]

coverage: 97.299% (-0.007%) from 97.306% when pulling e07577feab4ee3245280928bbdae91dd7eb1a891 on dan-da:195_bfe_not_canonical_pr into 8bc05393cefed3a44744b9193adbae442853bfa2 on Neptune-Crypto:master.

[jan-ferdinand]

Note that it is still possible to instantiate a Digest from BFieldElements that were created from non-canonical u64. Maybe that's ok...?

It's certainly okay if one has the BFieldElements themselves, as they take care of what appears to be duplicates in u64-land. It can start being weird if one were to use BFieldElement::from(my_u64). However, I think it's fine because a) this route seems convoluted and b) if one starts using BFieldElements it can be expected from them to understand what they are. In contrast, having to understand BFieldElement only because one wants to get a Digest from a &[u64] seems wrong.

I understand why this does not seem like a clean solution. I'm a little torn myself – following the principle of least surprise,

• someone who does not know what a finite field is might find it odd that $18446744069414584363 = 42$, while
• someone who does know what a finite field is will find is odd were $18446744069414584363 \neq 42$.

How can these two realms be married? I think the suggested solution does an acceptable job as outlined above.

[jan-ferdinand]

Digest also has a TryFrom BigUint. I looked at it, but it wasn't immediately clear to me if a canonical check is needed or exactly how to do it, so I didn't touch it. That might be another leak.

I'm actually not sure whether we're using BigUint anywhere in our stack. Maybe @Sword-Smith or @aszepieniec happen to know?