feat: implement bls key generation #86
Conversation
cmd/babylond/cmd/testnet.go
Outdated
| @@ -159,7 +163,8 @@ func InitTestnet( | |||
| return err | |||
| } | |||
|
|
|||
| nodeIDs[i], valPubKeys[i], err = genutil.InitializeNodeValidatorFiles(nodeConfig) | |||
| //accInfo, err := clientCtx.Keyring.KeyByAddress(clientCtx.FromAddress) | |||
| nodeIDs[i], valKeys[i], err = datagen.InitializeNodeValidatorFiles(nodeConfig, []byte("hello world")) | |||
There was a problem hiding this comment.
What is the "hello world" for? It looks like something that should not remain in production; a comment would not be amiss to explain.
There was a problem hiding this comment.
It looks to me that "hello world" should fail to validate when the validator tries to register their key with Babylon, because there we will use the account key that signed the transaction to validate the Proof-of-Possession.
There was a problem hiding this comment.
Yeah, "hello world" definitely does not work. I was using it as a placeholder and I forgot to replace it with the account key. Now I have fixed it. Thanks for pointing that out.
| } | ||
|
|
||
| func GenPrivKey() PrivateKey { | ||
| return genPrivKey(rand.Reader) |
There was a problem hiding this comment.
Ah good, for a moment I thought this was from the basic pseudo random package, not crypto/rand 👍
| } | ||
|
|
||
| // GetAddress returns the address of the validator. | ||
| // Implements PrivValidator. |
There was a problem hiding this comment.
Maybe there could be that interface assignment trick after the declaration of WrappedFilePV as well to make sure it's implemented?
There was a problem hiding this comment.
Do you mean that it would be better for WrappedFilePV to implement PrivValidator? If so, I'm not sure it is helpful to us because it is the internal logic of Tendermint, see here. We don't need to implement SignVote and SignProposal. But we do need to implement stuff like SignBLS. This will be implemented in a future PR.
There was a problem hiding this comment.
Right, thanks, I didn't know it's not implementing the full interface.
privval/types.go
Outdated
| type ValidatorKeys struct { | ||
| ValPubkey tmcrypto.PubKey | ||
| BlsPubkey bls12381.PublicKey | ||
| Pop *types.ProofOfPossession |
There was a problem hiding this comment.
| Pop *types.ProofOfPossession | |
| PoP *types.ProofOfPossession |
A bit more intuitive?
There was a problem hiding this comment.
Yeah, it's better. Thanks!
privval/types.go
Outdated
| // BuildPop builds a proof-of-possession by encrypt(key = BLS_sk, data = encrypt(key = Ed25519_sk, data = Secp256k1_pk)) | ||
| // where valPrivKey is Ed25519_sk, accPubkey is Secp256k1_pk, blsPrivkey is BLS_sk | ||
| func BuildPop(valPrivKey tmcrypto.PrivKey, blsPrivkey bls12381.PrivateKey, msg []byte) (*types.ProofOfPossession, error) { |
There was a problem hiding this comment.
Would it be possible to use an actual account public key type instead of msg []byte, as the comment describes?
There was a problem hiding this comment.
Yes, we can do it. Thanks!
| privKey := tmed25519.GenPrivKeyFromSecret([]byte(mnemonic)) | ||
| blsPrivKey := bls12381.GenPrivKeyFromSecret([]byte(mnemonic)) | ||
| filePV = privval.NewWrappedFilePV(privKey, blsPrivKey, pvKeyFile, pvStateFile) |
There was a problem hiding this comment.
Is there are risk of overriding an existing private key with a new one with a valid, but different mnemonic, thus losing the keys?
There was a problem hiding this comment.
Yes, it will overwrite the old keys when a new key is generated. But it's for testing only. Do we need to worry about a key backup in MVP?
There was a problem hiding this comment.
It didn't look like it was here to support tests, more like it's accidentally under the testutil package, thanks for pointing that out. Because it's called from cmd/babylond/cmd/testnet.go it looks like an actual thing that could be used in production as well. At least there was nothing to suggest that wasn't the idea.
testutil/datagen/init_val.go
Outdated
|
|
||
| // InitializeNodeValidatorFiles creates private validator and p2p configuration files. | ||
| func InitializeNodeValidatorFiles(config *cfg.Config, msg []byte) (string, *privval.ValidatorKeys, error) { | ||
| return InitializeNodeValidatorFilesFromMnemonic(config, "", msg) |
There was a problem hiding this comment.
I don't understand the empty mnemonic. According to https://github.com/babylonchain/babylon/pull/86/files#diff-954502830d7f26453fec2475dc8a85e43ffc312df8151a4745ad0be244dfbbfaR43 that will result in no BLS keys? What's the point of that?
There was a problem hiding this comment.
Ah, okay, it does generate BLS but not from a mnemonic. It looked confusing that one arm says LoadOrGenFilePV and the other says NewWrappedFilePV, like the first option wasn't wrapped.
There was a problem hiding this comment.
Yeah, it is confusing. Sorry about that. I have changed the function name
|
Thanks, @aakoshh for your comments. I have added account key into the generation of PoP. Please see the updates. The account key is generated via |
In Cosmos, a node generates all the genesis keys (including the account's key and validator's key) and stores keys in a config file before it starts running. In this PR, we added BLS key generation along with all the above keys. In particular, this PR includes: