-
Notifications
You must be signed in to change notification settings - Fork 67
[crypto] Switch signature scheme to BLS + Add Networking key. #857
Conversation
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.
Awesome! Don't forget to add a meaningful commit msg when merged
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.
This is a earth-shaking change for our code bases, changing the signature scheme from Ed25519 to BLS, and should be managed as such.
- Please add a meaningful commit message
- please label as breaking, and announce the breakage where appropriate,
- 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,
- 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.
Align w Francois, had the impression that whatever prior test was done already
@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. |
0d1fbe1
to
763d1a9
Compare
f990566
to
9a3fbc0
Compare
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.
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!
fe86468
to
ac5a371
Compare
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.
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, |
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.
Awesome!
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.
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?
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.
Fantastic!
…Labs/narwhal#857) * [crypto] switch to BLS * [crypto] network changes
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)