-
Notifications
You must be signed in to change notification settings - Fork 4
Bump synedrion to 0.3.0 and use manul for protocol session loop #1392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@fjarri @dvdplm i am a bit stuck on how to do the serialization of the protocol messages.
I have tried my best to copy what you did with the serializer for My code: entropy-core/crates/protocol/src/lib.rs Lines 127 to 167 in 1ed6627
But it seems to me like i don't have a example of how the serialization is used - in the examples messages don't actually go over the wire so they never need to be serialized / deserialized: https://github.com/entropyxyz/manul/blob/4f269080775aa6359d0d26d8b56940b4429882f7/manul/src/dev/run_sync.rs#L192-L221 |
Should be fixed by entropyxyz/manul#98 |
Thanks @fjarri that works - as in things now compile (tests not yet passing). |
It seems that for both the 'key init' and 'reshare' protocols, messages do get exchanged but then the session hangs before the round is finalized. Eg: Here is the key init protocol running with two parties (the first stage of a DKG with 3 parties):
At this point it hangs. |
|
Current state - most tests passing - but the entropy-tss jumpstart and reshare tests fail. |
* peg/fix-code-changed-check: Add check for the first commit of a branch
* peg/fix-code-changed-check: remove GITHUB_ENV redirect to simplify job
use crate::{protocol_message::ProtocolMessage, KeyParams, PartyId}; | ||
|
||
#[derive(Debug, Error)] | ||
pub enum GenericProtocolError<Res: ProtocolResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get rid of this error type and just use ProtocolExecutionErr
(below) - as things are simpler now.
|
||
/// This is used to run the synedrion protocols - it is mostly copied from the synedrion integration | ||
/// tests | ||
mod synedrion_test_environment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankfully we don't need this anymore as we can now use manul::dev::run_sync
which gives us the same thing.
Synedrion 0.3.0 and manul 0.2.0 have been released - this PR updates to using the new versions.
manul
now gives as a function which implements the protocol execution loop. Which means we can move it out of entropy-protocol and use themanul
-provided one. This is good as less code for us to maintain. However there are a couple of reasons why it is not a drop-in replacement:par_run_session
function takes atokio::sync::mpsc::unbounded::Sender
but we have atokio::sync::broadcast::Sender
ProtocolMessage
needs to be replaced withmanul::session::tokio::MessageIn
andmanul::session::tokio::MessageOut
- but we would lose the extra bits protocol message gives us (eg: being able to pass the verifying key)We get around this by using extra tasks/channels to pass messages into the manul execution loop. This feels a bit ugly but i don't think it has a big performance cost, so i am happy with it for now.
Other issues:
TestParams
and switched to always using the production parameters in tests. This will increase our CI time - but i think not by much as many of our tests already used production parameters. If people think its important i can make a followup putting back the test params - i think we just to addelliptic-curve
as a dependency toentropy-protocol
.SessionReport
with more details about protocol errors which are useful for slashing. But we don't yet do anything with it. I propose to do that in a follow up.