Fix Digest hex encoding to preserve numeric ordering

[haoshengzhen]

Summary

Fixes #214.

This changes Digest hex formatting/parsing to use the same numeric representation as BigUint conversion and Ord, padded to the fixed digest byte width.

Previously, small digests such as Digest([14, 0, 0, 0, 0]) formatted with the non-zero byte at the beginning of the hex string. With this change, the hex representation has leading zeroes and matches the digest's numeric value.

Changes

• Format Digest as fixed-width big-endian hex via BigUint::from(digest).
• Parse hex strings as fixed-width big-endian integers and convert them back into Digest.
• Add regression/property tests for:
  • leading zeroes for small digests
  • agreement with BigUint
  • agreement between lexicographic hex ordering and Digest::Ord
  • reciprocal to_hex / try_from_hex
• Update snapshot strings whose values depend on Digest::to_hex.

Verification

• cargo test --workspace --lib --tests
• cargo test --doc
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all -- --check

[Sword-Smith]

Do we want this? In isolation I think it makes sense. I'm just not sure it's worth it dealing with the fallout. I'm leading towards a yes. But maybe, for downstream compatibility there should be an option to have the small-endian version also?

Edit: Seems like more than just endianness is changing. I think it has to do with how the number are divided and doesn't necessarily poiint to an error in this PR.

[haoshengzhen]

Do we want this? In isolation I think it makes sense. I'm just not sure it's worth it dealing with the fallout. I'm leading towards a yes. But maybe, for downstream compatibility there should be an option to have the small-endian version also?

Edit: Seems like more than just endianness is changing. I think it has to do with how the number are divided and doesn't necessarily poiint to an error in this PR.

I think your edit is right: this is not only an endianness change.

The old implementation encoded the byte representation of the underlying digest elements. This PR instead makes the hex string represent the same numeric value used by BigUint::from(Digest) and by Digest::Ord. So the changed snapshots are expected: they are not just byte-order reversals, because the digest-to-number mapping also depends on how the field elements are combined.

My motivation was to address #214 as written: make to_hex, try_from_hex, Ord, and BigUint agree, and make small digest values have leading zeroes in hex.

I agree that the compatibility concern is the main tradeoff here. If downstream users may rely on the previous byte-oriented encoding, I can add explicit legacy helpers, for example to_hex_legacy / try_from_legacy_hex, or whatever naming you prefer. That would let the default to_hex be the canonical numeric representation while still giving downstream users a migration path.

[coveralls]

coverage: 97.847% (-0.09%) from 97.937% — haoshengzhen:master into Neptune-Crypto:master

[Sword-Smith]

I say we go with this and deal with the downstream problems when they appear. I foresee some problems downstream in neptune-core with both program digests and block hashes.

@haoshengzhen Do you think you can sign your commits such that this can be merged?

[Sword-Smith]

@haoshengzhen Can you sign your commits? Then we can merge.

[haoshengzhen]

@haoshengzhen Can you sign your commits? Then we can merge.

@Sword-Smith Sorry for the late reply.
Signed. Please review it again.

[Sword-Smith]

@haoshengzhen Just had a talk about this in the office. @aszepieniec will have the final say since he invented the TIP5 hashing algorithm. I'm sorry for the confusion.

[aszepieniec]

If I understand correctly, the only changes are user-facing displays (nothing mechanical); and these display changes are divisible into two separate points:

1. Use most-significant digit first instead of least-significant digit first digit ordering.
2. Pass through a big integer representation on the way to producing a hex string, instead of representing each digit separately.

I actually oppose both but for different reasons.

Big integer representation

There are many ways to embed arrays of 5 BFieldElements into the integers. Any base $\geq p$ will do. From the perspective of conversion to big integers, the code currently uses base $2^{64}$ and this PR proposes to drop that base to $p = 2^{64} - 2^{32} + 1$. I fail to see an advantage.

Converting base $p$ is more expensive than base $2^{64}$, which comes for free.

Converting base $p$ means implementers of Tip5 in other languages (or even re-implementers in rust) must also implement the display logic prior to validating their implementation against the given test vectors. This extra stage does not just raise the bar to getting feedback about the implementation's correctness; it also introduces more bug surface area because now the implementer has to deal with big integer arithmetic. In contrast, base $2^{64}$ comes for free.

Most-significant digit first

We inherit the digit order we use from numbers in prose from Hindu scribes who did not know about arrays and indexing. Given that the BFieldElements live in an array, the natural order of digits is the order in which they appear in the array. If you are looking for a digest and know the the value of the first BFieldElement, you will be looking for that value on the left side.

I am aware of the motivation for block hashes to appear small when they satisfy the proof-of-work threshold digest < target. The number of leading zeros (as opposed to the number of trailing zeros) gives an idea of how much work a block represents.

IMHO this application is rather artificial. It is equally possible to define Nakamoto consensus by counting leading (or trailing) ones instead. That's not how we do it in Neptune Cash, but that's a design choice that one validates by reading neptune-core, not twenty-first. In other words, twenty-first makes abstraction of whether or how Digests are used in consensus mechanisms. This PR erases that abstraction and bakes in specific assumptions in its place.

If we want block hashes of blocks that satisfy the threshold inequality to be displayed with leading zeros instead of trailing zeros (I am indifferent to this particular motivation) then the right place to write that display implementation is neptune-core, for instance as a trait implementation for newtype BlockHash.

Conclusion

I'm obviously grateful that a volunteer chose to donate time and effort to make a PR and I certainly don't want to be discouraging. That said, I do not think these particular changes improve the code base. If you want to keep focus on the feature you are targeting, small block hashes, then I recommend making a PR to neptune-core instead.