feat: add genesis state for checkpointing #101
Conversation
There was a problem hiding this comment.
Cool, thanks for opening a different PR with tests for moving into the direction we discussed!
I sent some comments; to summarise I think that we should:
- try to make the API symmetric and ask for both public keys during validation
- not rely on the key included in the PoP for validation, but rather look it up in the system based on where we are
- have dedicated type in the genesis config rather than have to validator fields during normal registrations that provide the same content
x/checkpointing/keeper/gen_bls.go
Outdated
| } | ||
| ok := blskey.Pop.IsValid(*blskey.Pubkey) | ||
| ok := blskey.Pop.IsValid(*blskey.Pubkey, valPubkeys[i]) |
There was a problem hiding this comment.
Maybe it would be worth doing an explicit assertion that there are equal number of items in both arrays.
x/checkpointing/keeper/msg_server.go
Outdated
| valAddr, err := sdk.ValAddressFromBech32(msg.MsgCreateValidator.ValidatorAddress) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| valPubkey, found := m.k.GetValidatorPubkey(ctx, valAddr) | ||
| if !found { | ||
| return nil, errors.New("can not find validator by address") | ||
| } |
There was a problem hiding this comment.
Aren't we just trying to create the validator after this, if the Proof-of-Possession is correct?
There was a problem hiding this comment.
Public key is in the message.
There was a problem hiding this comment.
I keep getting confused by these keys.
We said we can remove the account key from the PoP because the validator key will be used to sign the MsgCreateValidator message, right?
- Here it shows that anything used to sign a transaction is an account key: https://github.com/cosmos/cosmos-sdk/blob/a2761bdc6f23500a2b38d398ee2a423f2a5311c0/x/auth/ante/sigverify.go#L251-L258
- Here it shows that the delegator and the validator address must be the same: https://github.com/cosmos/cosmos-sdk/blob/a2761bdc6f23500a2b38d398ee2a423f2a5311c0/x/staking/types/msg.go#L67-L78
- So they say which account delegates
- There must be a
pubkeyas well, but it's not used to sign the message - The
pubkeyis checked to make sure it's not taken yet: https://github.com/cosmos/cosmos-sdk/blob/a2761bdc6f23500a2b38d398ee2a423f2a5311c0/x/staking/keeper/msg_server.go#L48-L50
So, if Eve sees Bob's MsgCreateValidator transaction, she can still snatch the PoP from it if she wants to - just as she could register the pubkey for herself, although she would not be able to use it for validation.
In the end I still got it wrong when I said in your earlier PR that it was safe as PoP only because the Ed25519 is used to sign the transaction, because it's not used like that. I guess we just have to make sure that nobody else can register the BLS key for themselves, unless we want to use the Secp256k1 key which is used to signed the transaction.
So the PoP proves not that the bearer possesses both BLS and Ed25519 keys, but rather that there is some person out there who wanted to use these keys together, but we don't know who they are and they can't prove it to Cosmos either, because Cosmos is not interested in signatures with these keys over any content that verifyable belongs to the sender 😵
|
Hi @aakoshh, thanks for your comments. Sorry for the early commit that caused some confusion as I was still revising the code. Now I think I have addressed all your comments. Regarding PoP, I think the current implementation is safe because each ed25519-bls binding is also bonded with an account address, the delegator. Therefore, the attacker can not register the same ed25519-bls binding with another account. This PR failed the integration test but I cannot find the cause. @KonradStaniec Could you please take a look? It seems that the testnet cannot be started but my local |
|
Hey @gitferry , so you can start local testnet on your local machine by When i try to do it on this branch the testnet fails to start due to some panic: My guess is that pr changes something with genesis handling which makes it impossible to start node. |
|
Thanks @KonradStaniec! This is super helpful. |
Right, so what we want to avoid is somebody registering a BLS public key that they don't own, which would allow them to mount an attack (although I forgot how). We do this by only allowing the first registration for the BLS key to go through. What we are not doing is actually verifying that the person sending the registration has the BLS private key, because they could just copy someone else's PoP and use it for themselves. But if they do so, the system would make sure they are the only ones with this now useless BLS key. So I agree that it's safe, it's just a bit confusing. But so was the basic idea of "PoP means you sign the BLS public key with the BLS private key" - anyone can steal that once they see it. |
| @@ -30,6 +31,18 @@ func (k Keeper) GetValidatorSet(ctx sdk.Context, epochNumber uint64) types.Valid | |||
| return types.NewSortedValidatorSet(vals) | |||
| } | |||
|
|
|||
| func (k Keeper) GetValidatorPubkey(ctx sdk.Context, valAddr sdk.ValAddress) (cryptotypes.PubKey, bool) { | |||
There was a problem hiding this comment.
No, I'll remove it.
| @@ -216,7 +216,15 @@ func InitTestnet( | |||
| if err != nil { | |||
| return err | |||
| } | |||
| valPubkeys = append(valPubkeys, valPubkey) | |||
| genKey := &checkpointingtypes.GenesisKey{ | |||
| ValidatorAddress: sdk.ValAddress(addr).String(), | |||
There was a problem hiding this comment.
this will create bech32 encoded string , while in https://github.com/babylonchain/babylon/pull/101/files#diff-5e33f2573bcc600876bb7e96e8241263ea8790b62060ce20a43e2a0ff35e538fR11 you expect hex-encoded.
Maybe it is worth using betting typing in GenesisKey struct or at least at some comments what is expected format of address string. I would expect bech32, as it is standard across cosmos-sdk to use bech32 encoded strings.
Also question, why do we have address string and ValPubkey *ed25519.PubKey in GenesisKey struct, when you can derive address from public key ?
There was a problem hiding this comment.
Also question, why do we have address string and ValPubkey *ed25519.PubKey
This is endlessly confusing, but based on the MsgCreateValidator my impression is that the address string is the account key (Secp256k1) and is not the same as the validator key. I thought that's what we have in genesis.proto as well: who is the account (validator_address) that paid to become a validator and what validator key (val_pubkey) and what BLS key they will use. We do this because genutil only accepts MsgCreateValidator which also has validator_address and delegator_address which have to be identical account keys, plus it has the pubkey to associate with them.
There was a problem hiding this comment.
Thanks @KonradStaniec, I have changed it to bech32.
There was a problem hiding this comment.
Also question, why do we have address string and ValPubkey *ed25519.PubKey in GenesisKey struct, when you can derive address from public key ?
The validator address actually is not from the validator's public key but from the account's public key. See here Yeah, it is confusing.
I think addresses from either the account public key or the ed25519 public key can both work.
cmd/babylond/cmd/testnet.go
Outdated
| Pubkey: &valKeys[i].BlsPubkey, | ||
| Pop: valKeys[i].PoP, | ||
| }, | ||
| ValPubkey: nil, |
There was a problem hiding this comment.
You are passing here nil so the BlsKey.Pop.IsValid(*key.BlsKey.Pubkey, key.ValPubkey) will fail in https://github.com/babylonchain/babylon/pull/101/files#diff-5e33f2573bcc600876bb7e96e8241263ea8790b62060ce20a43e2a0ff35e538fR19
There was a problem hiding this comment.
Thanks for pointing that out. I have fixed it but I still get nil err when I ran make localnet-start, it seems that valKey is still nil. I think the reason is because unmarshalling json to Pubkey does not succeed because Pubkey is Any type and the unmarshalling looses the cachedValue.
gitferry
force-pushed
the
checkpointing/add-genesis
branch
2 times, most recently
from
f8452cf
to
d764ce5
Compare
gitferry
force-pushed
the
checkpointing/add-genesis
branch
from
d764ce5
to
82360a0
Compare
This PR adds genesis state for the checkpointing module. In particular, it adds Bls keys and pop into
GenesisStateof the checkpointing module and implementsInitGenesis.