Skip to content
This repository has been archived by the owner on Oct 17, 2022. It is now read-only.

[crypto] Switch signature scheme to BLS + Add Networking key. #857

Merged
merged 2 commits into from
Sep 12, 2022
Merged

Conversation

punwai
Copy link
Contributor

@punwai punwai commented Aug 26, 2022

This PR switches the signature scheme for signing consensus transactions to aggregatable BLS12-381 signatures. That said, we now need the validators to own another ed25519 key, NetworkKeypair, so that a QUIC connection can be established. Moreover, as the worker's only need their keypairs to interact with the primary, we switch the worker's keypair from BLS12381 to Ed25519.

Must be followed by:
MystenLabs/sui#4408 (sui)
https://github.com/MystenLabs/sui-operations/pull/366 (production configs)

kchalkias
kchalkias previously approved these changes Aug 26, 2022
Copy link
Contributor

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

Awesome! Don't forget to add a meaningful commit msg when merged

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This is a earth-shaking change for our code bases, changing the signature scheme from Ed25519 to BLS, and should be managed as such.

  1. Please add a meaningful commit message
  2. please label as breaking, and announce the breakage where appropriate,
  3. Please coordinate with the Sui folks, as this will break NW's ability to receive an Ed25519 key, hence we need the switch to BLS ready there as well,
  4. please check the keytool there (sp. for any PoP generation), sui-operations (both Sui and Narwhal configs therein), sui-genesis, so that they are all ready to receive a switch to BLS at the drop of a hat.

@kchalkias kchalkias dismissed their stale review August 27, 2022 01:11

Align w Francois, had the impression that whatever prior test was done already

@punwai
Copy link
Contributor Author

punwai commented Aug 27, 2022

@huitseeker @kchalkias Sorry for the confusion, I was supposed to open up this PR as a draft. I have this branch opened so that I have a remote commit hash to import into Sui, for the purpose of developing a BLS PR there. I did not intend for this PR to be merged. That said, these are super helpful comments and I will ensure to address this moving forward.

@punwai punwai changed the title f [crypto] Switch signature scheme to BLS Aug 27, 2022
@punwai punwai changed the title [crypto] Switch signature scheme to BLS [crypto] Switch signature scheme to BLS [WIP] Aug 27, 2022
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thanks a lot for the huge amount of work it takes to bring this over the finish line!

I have only one remark : switching the key type in the WorkerInfo to a NetworkPublicKey. Otherwise, this LGTM!

config/src/lib.rs Show resolved Hide resolved
crypto/src/lib.rs Show resolved Hide resolved
@punwai punwai changed the title [crypto] Switch signature scheme to BLS [WIP] [crypto] Switch signature scheme to BLS Sep 9, 2022
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thank you so much @punwai! 🎉

@@ -299,7 +299,7 @@ impl Parameters {
#[derive(Clone, Serialize, Deserialize, Eq, Hash, PartialEq, Debug)]
pub struct WorkerInfo {
/// The public key of this worker.
pub name: PublicKey,
pub name: NetworkPublicKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

Copy link
Contributor

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

IMO we should have split this PR into "adding Network Key" and "switch to BLS". If we merge it as a single PR (it's fine), let's please update the PR's title + ensure that the merged commit message reflects both changes.

Also, there is a pending MystenLabs/sui PR to switch to BLS there as well, is there a coordination effort before merging both?

benchmark/benchmark/config.py Show resolved Hide resolved
@punwai punwai changed the title [crypto] Switch signature scheme to BLS [crypto] Switch signature scheme to BLS + Add Networking key. Sep 11, 2022
Copy link
Contributor

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

Fantastic!

@punwai punwai merged commit e40e37a into main Sep 12, 2022
@punwai punwai deleted the bls branch September 12, 2022 08:27
@huitseeker huitseeker mentioned this pull request Sep 12, 2022
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants