Skip to content
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

Update Synedrion usage to v0.0.12 #525

Closed
wants to merge 2 commits into from

Conversation

fjarri
Copy link
Member

@fjarri fjarri commented Nov 23, 2023

  • 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).

Copy link

vercel bot commented Nov 23, 2023

@fjarri is attempting to deploy a commit to the entropyxyz Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Nov 23, 2023

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

Name Status Preview Comments Updated (UTC)
entropy-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 9:07pm

@ameba23
Copy link
Contributor

ameba23 commented Nov 24, 2023

I don't think it will resolve this issue completely, but i am planning to ditch subxt_signer now that we figured out how to use sp_core::sr25519 on wasm. So it will go back to being Public rather than PublicKey. Which i guess has a From <AccountId32> but don't know how much that will help. I can try to get this done today unless it turns out to be a complicated one.

@fjarri
Copy link
Member Author

fjarri commented Nov 26, 2023

Not sure why the tests are not being run.

let mut accum = session.make_accumulator();

// Send out broadcasts
let destinations = session.broadcast_destinations();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are broadcast destinations sometimes different on different rounds? As in, does this need to be called inside the round loop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the moment, no. But compared to other parts of the loop it takes negligible time to generate that vector, and I didn't want to want to introduce additional state that the user needs to maintain. Moreover, with entropyxyz/synedrion#62 the broadcast part will go away anyway, there will only be direct messages left.

@ameba23
Copy link
Contributor

ameba23 commented Nov 27, 2023

Cool! This also fixes #416

And if i've understood right, we no longer rely on array indexes to identify parties, so no more issues around ordering of account IDs.

@fjarri
Copy link
Member Author

fjarri commented Nov 27, 2023

There is still some ordering exposed, since there is a fixed order of key shares in KeyShare, and the order of PartyIds given to the session constructor must match it. I cannot include PartyId in the share itself since the verifiers are external to Synedrion, plus as far as I understand we want one share to be kept on multiple nodes.

Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. How did you get CI to run in your fork in the end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to a tagged release of synedrion, not a branch with a fix Refactor pre protocol execution
2 participants