Make supplying a serializer the user's responsibility

[fjarri]

Fixes #26

The problem here is somehow bringing the serializer through the object-safe API (of ObjectSafeRound) so that the user could call it in the methods of Round responsible for handling messages.

What we want is for the user to provide two methods

fn serialize<T: Serialize>(value: T) -> Result<Box<[u8]>, LocalError>;
fn deserialize<'de, T: Deserialize<'de>>(bytes: &'de [u8]) -> Result<T, DeserializationError>;

and pass those methods to methods of Round somehow.

It is possible to do with serialize, via erased-serde: in Round::make_direct_message() we wrap some Message: Serialize into Box<dyn erased_serde::Serialize> and pass to the session level, which can apply the user-provided serialize to it. But it seems to be impossible to do with deserialize, unless the user can instead provide an object implementing serde::Deserializer, which can then be wrapped in Box<dyn erased_serde::Deserializer> and passed to Round::receive_message. Unfortunately, most popular libraries do not expose such an object (a rare example of a library that does is serde_json).

This PR introduces a Format trait, and a type of that trait in SessionParameters. Now it can be either made an additional generic parameter of Round - the downside is a more complicated API for both protocol writers and protocol users. Or we could go the Box<dyn erased_serde::Deserializer> way, make a PR to the libraries we plan to use exposing the Deserializer, and let users who want other formats deal with format implementors themselves. This PR implements the latter.

TODO:

• figure out where exactly to put format.rs, since Format itself should be exported in manul::session, but Serializer/Deserializer should be exported in manul::protocol.

[fjarri]

@dvdplm This PR is kind of experimental, so I would like to know your opinion on

• whether the linked issue is worth fixing, and
• which of the two approaches you think is better (or maybe you have other ideas)

[fjarri]

Alternatively, we could go the Box<dyn erased_serde::Deserializer> way

With this option, I'm currently stuck at trying to figure out a way to pass the type-erased deserializer to Round. See dtolnay/erased-serde#107 for the description of the problem.

[coveralls]

Pull Request Test Coverage Report for Build 11587348171

Details

• 210 of 249 (84.34%) changed or added relevant lines in 11 files are covered.
• 2 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+1.4%) to 72.149%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
manul/src/session/echo.rs	8	9	88.89%
manul/src/session/session.rs	49	50	98.0%
manul/src/protocol/round.rs	5	7	71.43%
manul/src/testing/macros.rs	22	31	70.97%
manul/src/testing/wire_format.rs	13	25	52.0%
manul/src/session/evidence.rs	15	29	51.72%

Files with Coverage Reduction	New Missed Lines	%
manul/src/session/evidence.rs	1	42.09%
manul/src/testing/macros.rs	1	85.05%

Totals
Change from base Build 11563502171:	1.4%
Covered Lines:	1474
Relevant Lines:	2043

---

💛 - Coveralls

[fjarri]

Update: implemented dynamic (de)serializers. The downsides are the following:

• We need the chosen format to expose a type implementing serde::Deserializer. Fortunately, there is already a binary format that does that, postcard, which we can use for tests. serde_json exposes it too. We can make PRs to bincode and rmp-serde, if this PR goes in.
• There is a performance regression, as expected. 7% for the no-echo benchmark, and 22% for the echo benchmark. Noticeable, but not too bad overall, they're still quite fast - we're targeting heavy cryptographic protocols, after all. (master uses bincode for tests, while this PR uses postcard, but they have the same performance, for empty messages at least).
• Anything that creates a Deserializer needs to add the bound for<'a> ... from Format::deserializer(). That's because Rust cannot propagate higher-ranked trait bounds yet (see Higher-ranked trait bounds on associated types are not elaborated rust-lang/rust#50346). Not sure if it can be solved by somehow encapsulating this bound in SessionParameters...

The performance can be improved if we use static deserialization on the session level, but that requires writing down the scary bound described above in more places.

The alternative to all that is parametrizing Round with F: Format. Is the additional generic noise worth it?

[dvdplm]

This is tricky. My gut tells me that this shouldn't be this hard and that we're missing something. I'm not saying this as a critique of this PR specifically: it was complex before as well.

Are we applying the wrong abstraction? Are we missing a layer somewhere, something like a super-session object that orchestrates the protocol, the wire format and session params? Should ser/deser even be a concern for manul at all?

Besides these philosophical concerns I think this PR improves some code and worsens other parts. Lugging around extra arguments is noisy but we gain some of that back from having a simpler setup and a better isolation in the form of the Format trait. I think maybe there's some polishing possible here and there, but my biggest concern is that the type erased stuff is complex. Is it too complex though?

Overall I don't think my concerns are blockers for merging – it wasn't pretty before and it isn't pretty now – but I think we should open issues with bincode and rmp-serde to check if they would accept PRs.

[fjarri]

This is tricky. My gut tells me that this shouldn't be this hard and that we're missing something. I'm not saying this as a critique of this PR specifically: it was complex before as well.

I personally blame serde for having people impl Deserializer on &mut instead of on objects themselves (and Rust for having weird lacunas in its type system). Hard to say why this choice was made, there may or may not be some discussion about it early in serde PRs. I thought about making a PR to erased_serde to be able to Deserializer::erase() an object itself instead of its mutable reference, but, while reducing the required type magic, that wouldn't get rid of the pesky bound.

Should ser/deser even be a concern for manul at all?

We have to serialize messages to sign/verify them, so as long as that's a part of manul, serialization must be too. At least now the format is supplied from the proper level (the user).

Besides these philosophical concerns I think this PR improves some code and worsens other parts. Lugging around extra arguments is noisy but we gain some of that back from having a simpler setup and a better isolation in the form of the Format trait. I think maybe there's some polishing possible here and there, but my biggest concern is that the type erased stuff is complex. Is it too complex though?

It is complex, but that complexity is mostly hidden from the users. Completely hidden for the protocol writers, somewhat hidden for the session runners - if they fix SessionParameters they won't have to deal with the for<'a> bound, it'll only show up if someone writes code generic on SessionParameters. If we decide to go the static route, the protocol writers will now have to make everything generic on Format.

In any case I think the previous situation, where the format was the choice of the protocol, was unacceptable and had to be fixed in some way.

Overall I don't think my concerns are blockers for merging – it wasn't pretty before and it isn't pretty now – but I think we should open issues with bincode and rmp-serde to check if they would accept PRs.

Certainly, once this PR is in, I'll do that, unless you want to.

[fjarri]

So, the new version manages to avoid scary high-ranked bounds, and won't need too much changes in bincode to support it (and already works with postcard and serde-json). Had to write https://docs.rs/serde-persistent-deserializer for that.