Update Synedrion usage to v0.0.12 #535

fjarri · 2023-11-28T20:01:36Z

(had to move #525 from my fork to the main repo because CircleCI sucks)

Fixes Move to a tagged release of synedrion, not a branch with a fix #509
Fixes Refactor pre protocol execution #416
Public vs AccountId32 #376 is still open: even though we're using PublicKey instead of Public now, the problem still stands: we have AccountId32 and Public with identical contents.
VerifierWrapper was removed, PartyId now implements the necessary traits directly.
ProtocolMessage does not make a distinction between broadcast and direct messages anymore, since our messaging is direct-only. There is still a distinction in the protocol execution loop, but it will be removed when message bundling is implemented (see Message bundling synedrion#62)
Removed a check for messages sent to ourselves in the protocol loop because evidently it is now checked in protocol_transport/mod.rs
Generalized the three protocol loops.
Note that "my_idx" is not being passed anymore from the upper levels; this information is already present in the relation between the signer and the verifiers, and if the signer's key is not present among the verifiers, there will be an error on the session creation stage (synedrion::sessions::Error::Local).

vercel · 2023-11-28T20:01:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
entropy-core	🔄 Building (Inspect)	Visit Preview	Nov 28, 2023 8:01pm

ameba23

i already approved the PR from the fork version - assuming this is the same

HCastano

Mostly just questions since I'm not familiar with this stuff yet 😄

HCastano · 2023-11-28T20:48:48Z

crypto/protocol/src/execute_protocol.rs

-        .into_iter()
-        .map(|acc| VerifierWrapper(sr25519::Public(acc.0)))
-        .collect::<Vec<_>>();
+    let pair = PairWrapper(threshold_pair.clone());

    // TODO (#375): this should come from whoever initiates the signing process,
    // (or as some deterministic function, e.g. the hash of the last block mined)
    // and be the same for all participants.
    let shared_randomness = b"123456";


HCastano · 2023-11-28T20:49:11Z

crypto/server/src/helpers/user.rs

@@ -27,7 +27,6 @@ pub async fn do_dkg(
    signer: &PairSigner<EntropyConfig, sr25519::Pair>,
    state: &ListenerState,
    sig_request_account: AccountId32,
-    my_subgroup: &u8,


We'll need to remove this from the fields list above

HCastano · 2023-11-28T20:50:13Z

crypto/protocol/src/protocol_message.rs

        from: &PartyId,
        to: &PartyId,
        payload: SignedMessage<sr25519::Signature>,
    ) -> Self {
-        Self { from: from.clone(), to: Some(to.clone()), payload }
+        Self { from: from.clone(), to: to.clone(), payload }


I know you didn't introduce this, but it might be nicer to take owned values in the function and have the caller potentially allocate up-front

Even better, PartyId should just implement Copy.

HCastano · 2023-11-28T20:51:03Z

crypto/protocol/src/lib.rs

+    fn to_public(&self) -> sr25519::Public {
+        // TODO (#376): assuming that `Public` and `AccountId32` represent the same 32 bytes.
+        // Ideally we should use only one of those throughout the code, probably `Public`.
+        sr25519::Public(self.0 .0)


Bit of a random question, but do you know why this gets formatted like this instead of self.0.0?

HCastano · 2023-11-28T20:53:10Z

crypto/protocol/src/execute_protocol.rs

+        // Send out broadcasts
+        let destinations = session.broadcast_destinations();
+        if let Some(destinations) = destinations {
+            // TODO: this can happen in a spawned task


We should make an issue for this and either annotate the TODOs or get rid of them

HCastano · 2023-11-28T20:54:39Z

crypto/protocol/src/execute_protocol.rs

+                tx.send(ProtocolMessage::new(&my_id, destination, message))?;
+
+                // This will happen in a host task
+                accum.add_artefact(artefact)?;


Suggested change

accum.add_artefact(artefact)?;

accum.add_artifact(artifact)?;

Sorry, I was using British spelling here, which I thought was the only one, but only later realized that there are two variants. I don't mind switching to the American spelling, but I need to change it in Synedrion for consistency.

Gotcha, didn't realize there was a British spelling for this. For APIs though I'd stick to American English since that's the most common thing in tech

HCastano · 2023-11-28T21:09:39Z

crypto/protocol/src/execute_protocol.rs

-            Ok(())
-        } else {
-            Err(signature::Error::new())
+async fn execute_protocol_generic<Res: ProtocolResult>(


Could be useful to annotate this with #[tracing::instrument] and have some events throughout the execution flow

HCastano · 2023-11-28T21:12:08Z

crypto/protocol/src/execute_protocol.rs

+    loop {
+        let mut accum = session.make_accumulator();
+
+        // Send out broadcasts


Since I'm not familiar with this bit of the code, can you explain the difference between sending out and broadcast and sending direct messages and why both are needed?

The difference is basically that the broadcast message is prepared once for all destinations, and the direct ones are prepared for each destination separately. Since it takes some times to generate the proofs (and I wanted to leave a possibility of parallelizing the process), it is a meaningful distinction. It could also affect the way we send the messages out, but since we don't have access to a broadcast channel, it doesn't.

That said, I'm planning to simplify the API by only sending one message per destination - entropyxyz/synedrion#62

HCastano · 2023-11-28T21:14:57Z

crypto/protocol/src/execute_protocol.rs

+            accum.add_processed_message(processed)??;
+        }
+
+        while !session.can_finalize(&accum)? {


What does it mean for a session to get "finalized"?

Good question, perhaps a better name for this object is Round, since we create a new one for every round. If that is what you meant.

Or I suppose the method can be called finalize_round()

I was more asking about the conditions around a session/round being finalized, and any guarantees or assumptions that can come afterwards

Basically, at this stage you merge the info you got from all the other parties during the round. finalize() contains anything that needs more than just the initial state of the round and the message from one node. For example, summing up secret share changes coming from other nodes. If it fails, that's when the "correctness proofs" become necessary, because you can't pinpoint the malicious node, so you can only generate a proof that you yourself did everything correctly and publish it.

fjarri added 2 commits November 28, 2023 11:54

Update Synedrion usage to v0.0.12

2be3fd0

Generalize session running loop

edfee83

vercel bot deployed to Preview November 28, 2023 21:07 View deployment

ameba23 approved these changes Nov 28, 2023

View reviewed changes

fjarri merged commit bee9f71 into master Nov 28, 2023
10 checks passed

fjarri deleted the fjarri/update-synedrion branch November 28, 2023 21:12

HCastano reviewed Nov 28, 2023

View reviewed changes

HCastano changed the title ~~Fjarri/update synedrion~~ Update Synedrion usage to v0.0.12 Nov 30, 2023

ameba23 mentioned this pull request Dec 6, 2023

Don't broadcast p2p signing messages #200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Synedrion usage to v0.0.12 #535

Update Synedrion usage to v0.0.12 #535

fjarri commented Nov 28, 2023

vercel bot commented Nov 28, 2023

ameba23 left a comment

HCastano left a comment

HCastano Nov 28, 2023

HCastano Nov 28, 2023

HCastano Nov 28, 2023

fjarri Nov 28, 2023

HCastano Nov 28, 2023

HCastano Nov 28, 2023

HCastano Nov 28, 2023

fjarri Nov 28, 2023

HCastano Nov 28, 2023

HCastano Nov 28, 2023

HCastano Nov 28, 2023

fjarri Nov 28, 2023

HCastano Nov 28, 2023

fjarri Nov 28, 2023

fjarri Nov 28, 2023

HCastano Nov 28, 2023

fjarri Nov 28, 2023

	accum.add_artefact(artefact)?;
	accum.add_artifact(artifact)?;

Update Synedrion usage to v0.0.12 #535

Update Synedrion usage to v0.0.12 #535

Conversation

fjarri commented Nov 28, 2023

vercel bot commented Nov 28, 2023

ameba23 left a comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment