axelarnetwork / tofn

A threshold cryptography library in Rust
Apache License 2.0
112 stars 23 forks source link

Reduce boilerplate code in protocol implementations #136

Closed ggutoski closed 4 months ago

ggutoski commented 3 years ago

Based on a previous PR comment.

Two tasks:

  1. Executer API should give incoming messages as a VecMap of enum (bcast_and_p2p, bcast_only, p2p_only) for each player.
  2. Rounds should be able to specify at compile-time the subset of possible message types they can accept. eg. bcast_and_p2p or p2p_only but not bcast_only.
ggutoski commented 3 years ago

@milapsheth item 2 above doesn't sound so useful in combination with item 1. ie. Even if the SDK checks each player to see whether that player's message types are in the allowed set, the protocol implementer still needs to fill all match arms for the enum in the round implementations. So the implementer will still need a line of code to deal with disallowed message types. I guess the only difference is that now it's a TofnFatal instead of just a faulter, since the SDK should have already returned a faulter before you got here.

It will be a nontrivial amount of work to implement item 2 and it will add complexity to the implementar API. You still think it's worth it?

milapsheth commented 3 years ago

Hmm, I was thinking that the Executer API will remain the same for the type of bcasts_in and p2ps_in. So, for e.g., if the Round expects BcastsOnly, then it can convert the bcasts_in FillVecMap to a VecMap right away and return TofnFatal if it fails without doing any additional checks, while ignoring the p2ps_in argument altogether. This will remove both the validation loops from that Round. For more complicated scenarios, when you have two possible states for each peer, say BcastsOnly or P2psOnly depending on happy/sad path, the Round just needs to filter for peers who sent a P2p (and are in the sad path), and kick off to sad path, without checking that they also didn't send a bcast. I'm happy to take on item 2 and see if it's worth it.

ggutoski commented 3 years ago

In offline discussion we decided to investigate a solution that would follow the pattern of our existing blanket implementation of ExecuteRaw. That is, define new traits ExecuterBcastOnly, ExecuterBcastAndP2p, etc. For each such trait write a blanket implementation of Executer containing boilerplate code that would otherwise be duplicated across rounds. Then protocol rounds may implement one of these traits instead of Executer.

It works as expected for the first such trait implementation and I'm quite pleased with the result. Unfortunately, we run into a problem with multiple such traits. The code:

impl<T: ExecuterBcastOnly> Executer for T { /*...*/ }
impl<T: ExecuterBcastAndP2p> Executer for T { /*...*/ }

produces the build error:

conflicting implementations of trait `sdk::executer::Executer`

See the commit https://github.com/axelarnetwork/tofn/commit/5013ee099187451870483d72a3e9886258c14463 for details.

A potential solution is to somehow make ExecuterBcastOnly and ExecuterBcastAndP2p mutually exclusive, so that no type can implement both simultaneously and so the compiler should always be able to deduce which implementation of Executer to use. Indeed, it is possible to achieve mutual exclusivity via associated types. Unfortunately, this solution is thwarted by a long-standing Rust compiler bug: https://github.com/rust-lang/rust/issues/20400

A workaround is described in the following article: Mutually Exclusive Traits in Rust. But this workaround is quite ugly and I was unable to adapt it to our setting in a reasonable amount of time.

As such, it seems we must resort to a less elegant solution: provide a library of helper functions for boilerplate code. Each round should start by calling the appropriate helper function and return faulters if any were detected.

ggutoski commented 3 years ago

A failed attempt to use marker types to work around the problem of conflicting trait implementations: https://github.com/axelarnetwork/tofn/commit/7137933bf0053698b7014cd4ed650cd5e98ae3ce