Move ReprBytes32 to its own crate and generalize it

[cbeck88]

We currently have a lot of code that is like manually grabbing
prost::encoding::*; and using it to create custom serializations
with prost.

This is a hazard, because that is not part of the public-facing
interface of prost and it is not documented. It also changes
periodically as upstream refactors the library. The only reason
these symbols are publicly visible at all is that there is no way
that prost-derive can function if the proc-macro generated code
cannot access these symbols. When we grab prost::encodings::*;
it is creating a maintanence hazard that will make it very hard
ever to uprev prost again.

In the past, every cryptographic primitive that went into prost
or serde required custom bindings code, which was very long.
ReprBytes32 was a quick stopgap to prevent the code duplication
and take us down a route that would be more maintainable.

It turns out that we have some types that are not exactly 32
bytes, and then we have started grabbing the prost::encodings::*
again to support those.

Separately from this, we have had a problem in keys crate where
we need to be able to get the bytes of any Kex public key.
Previously, Kex was only implemented for X25519 and was not
implemented for Ristretto, which prevented us from writing generic
(curve agnostic) code in a lot of places.

In #74 we fixed
this, but we needed to introduce a trait GetBytes as a stopgap.

I propose that GetBytes can go away and be replaced with this
version of ReprBytes. I propose that ReprBytes32 can also go
away and be replaced with this version of ReprBytes.

I believe that ReprBytes should potentially be moved out of
mobilecoin repo and treated as a "fundamental trait" in the RustCrypto
ecosystem. Then, crates like prost and serde would integrate with
ReprBytes trait, and e.g. Dalek crates would not have to know
about prost or Dalek would not have to know about prost or
serde, they would only have to know about ReprBytes.

Even if RustCrypto folks don't want to take ownership of this,
this will reduce a lot of nasty code duplication in our repo now,
and may help to clean up the keys crate, and make it able to achieve
its goals of abstracting both X25519 and Ristretto.

---

Alternatives: In keys crate, the main alternative is, try to make everything implement AsRef<[u8]> and Into<Vec<u8>>. Because these are the "standard library interfaces" for doing this.
However, traits like

/// A trait which all public key structures should implement.
pub trait PublicKey:
    Clone
    + Debug
    + DeserializeOwned
    + Digestible
    + Eq
    + Hash
    + Into<Vec<u8>>
    + PartialEq
    + PartialOrd
    + Ord
    + Serialize
    + Sized
    + for<'bytes> TryFrom<&'bytes [u8]>
{
    /// Retrieve the size of this public key's raw bytes.
    fn size() -> usize;
}

Are bad because they force a requirement on extern crate alloc. Because they have a trait bound on a core trait Into<Vec<u8>>, there is no way that a consumer of this API can implement the trait unless they also have access to alloc crate.

Suppose we wanted to take keys crate and add an alloc feature, so that it can be used in microcontroller environments without a standard allocator. All existing elliptic curve implementations do not require an allocator, and all of the code in RustCrypto is friendly to no-alloc development.

If we try to simply do #[cfg(feature = "alloc")] to make the Into<Vec<u8>> go away, then we are creating global feature coordination problems. Anything that implements keys traits will have to have an alloc feature, and that feature must be configured exactly the same way as the way that keys crate is configured. This is exactly the same problem that serde has, and the source of an enormous number of build problems, for us, for months. This would not be an "additive" use of features.

Cargo expects / requires that when a crate has features, turning them on is "additive", only adding things to the API, and not removing them. Otherwise, cargo's global feature unification is unsound and causes build breakage. Making a trait bound stricter when a feature is turned on is not additive, and the fact that serde itself does this, is a bug.

(My criticism of serde, in this way, with respect to non-additive std::error::Error, is the reason that they added this release of serde: https://github.com/serde-rs/serde/releases/tag/v1.0.100. I think the reason that it ended up this way in the first place is that serde is older than no_std, and when they created no_std they failed / neglected to move std::error::Error to core.)

So I think that we should expect to redesign the keys crate so that it does not have any dependence on Into<Vec<u8>> if we expect that it will eventually be taken up by RustCrypto folks. I think that a trait like ReprBytes can be an adequate replacement for it.

[cbeck88]

Please give me feedback on the general direction here, I want to make sure we agree this is a good idea before I start trying to nuke ReprBytes32.

Please also review #74

[eranrund]

I'm not sure about the whole keys/GetBytes thing (I think @jcape would have valuable input on that), but as a generic solution for the custom serializations we currently have in a few places, this seems like a good alternative to me if it works (I'd like to see how it looks like when replacing one or two of the custom ones).
I can see this living under mcserial if the only usage ends up being the prost/serde macros.

[cbeck88]

@eranrund the ReprBytes32 lives under mcserial right now, but @jcape discussed in the past that that should not be there, because we want for the keys crate to have a nice future and mcserial is like an internal serialization API. The end goal for keys crate I think is that the stuff in traits.rs becomes a part of this repo: https://github.com/RustCrypto/traits just like the aead trait that James worked on. I would like that ReprBytes becomes not only the basis of these macros, but also is used in either the PublicKey or the Kex trait, so that we can implement Kex for both X25519 and Ristretto and get the byte representations in generic code

[cbeck88]

@eranrund I added tests that demonstrate this functionality, and I rebased the Ristretto Kex PR on this so we can see how it looks: #74

[eranrund]

This looks good to me when it comes to solving the serde/prost ugly implementations. I am not sure on its effects with regards to Kex stuff. This has my okay to replace ReprBytes32 is @jcape feels good about it.

[mfaulk]

This looks well thought-out. I've had good experiences with RepBytes32 and the convenience macros it enables, so I'm glad to see it maturing.

[cbeck88]

@jcape This is in limbo for almost a month

We've had some off-line discussions about how this could be redone to be nicer in the future.

There is no clearly scoped path forwards on that.

We should not let perfect be the enemy of good, we have to get things done. This PR has been very carefully thought out, and if not merged, this will be blocking things very soon.

I tried to create a large window of time for discussion and requesting of changes.

Unless you can have concrete changes to request, please don't block this PR any longer.

[jcape]

@garbageslam You promised to "circle back" on the in-place API, conducive to merging with mc_util_encoding::{FromX64, ToX64} multiple times, and never did. Eventually on May 1, I said I'd try to help out after I was done with the micro-architectural hardening, which turned out to be much tougher than anticipated, and continued until last Wednesday. I appreciate this PR has been sitting for a while, but I thought we had a direction for it.

I'm sorry I forgot to take this over last Wednesday, but I was reviewing your DB PR instead, because it seemed like that was a higher priority.

[jcape]

There's some cleanup to be done: basically anything that's using a core/alloc reference needs to get flipped into a re-export. I've opened MC-1474 to track integrating this with the mc_util_encodings::{ToX64, FromX64}, in the assumption this will be reworked later.

[cbeck88]

@jcape i wanted to avoid making prost and serde explicit dependencies of repr bytes, because that creates versioning problems. also they are opt in, the user of repr bytes need not depend on prost and serde if they don't want or don't need, they only get those if they use the macros.

lmk if you're sure that they should become hard dependencies of the lib, that seems to me like it might not be desirable. right now they are only dev-dependencies.

right now the crate is compatible with a core-only build. prost and serde deps probably won't work with that. we can add features to turn them on i guess.

[cbeck88]

if there is a use-case for having core-only targets and targets which use prost built from the same workspace, which both use this trait, then we should not make prost even an optional dependency of this lib

[cbeck88]

i guess if we have such a use-case, we can just split this crate into two crates, one with the traits, and one with the macros.

[cbeck88]

if we think this trait will eventually go away anyways then i'm not going to worry about that for now.

[cbeck88]

@jcape "You promised to "circle back" on the in-place API, conducive to merging with mc_util_encoding::{FromX64, ToX64} multiple times, and never did."

Yeah, sorry, I got over-burdened. I will help to try to merge with mc_util_encoding but I need to have a faster path to stabilize things based on generic kex traits so that this work does not block stabilizing those things. We should be able to rework the traits in a nonbreaking change that is not too disruptive

[jcape]

Looks great, couple things need fixing (feature gating the macros alongside the crates, and it looks like Cargo.lock got updated in a weird way---I've done the same thing multiple times, easy way to fix it is revert to the one in master, then run a build to catch intended changes...)