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

feat: define checkpointing registration proto and state #46

Merged
merged 7 commits into from
Jul 6, 2022

Conversation

gitferry
Copy link
Contributor

@gitferry gitferry commented Jul 5, 2022

This PR partly fixed BM-46.

In particular, this PR defined a wrapper called MsgWrappedCreateValidator that contains BLS public key and MsgCreateValidator of the Staking module.

The checkpointing module processes the wrapper to (1) pre-register the validator by its BLS public key, and (2) forward the corresponding MsgCreateValidator to the Staking module at the end of an epoch.

This PR fixed (1), and the following PR will fix (2).

@gitferry gitferry requested a review from aakoshh July 5, 2022 02:45
@gitferry gitferry force-pushed the gai/checkpointing-registration branch from c24295f to d78df0f Compare July 5, 2022 02:51
Comment on lines 17 to 18
// ed25519_pk is the Ed25519 public key of the validator
bytes ed25519_pk = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this in the message, or can we infer it from the transaction? Do validators sign transactions with their Ed25519 keys? I'm not sure now that @SebastianElvis pointed me at some Secp256k1 documentation as well.

Copy link
Member

Choose a reason for hiding this comment

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

The validator type includes the validator operator (i.e., the user)'s address (which has to be associated with a Secp256{k/r}1 key pair) and the validator's PK (which has to be associated with an Ed25519 key pair). Here perhaps we can find out the validator's Ed25519 PK from the CreateValidator message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my understanding, the transaction is signed by Secp256 instead of Ed25519. Ed25519 is only for consensus messages. I agree with @SebastianElvis here that the ed25519_pk is included in the MsgCreateValidator that is wrapped in the MsgWrappedCreateValidator, so I maybe can remove it from PoP.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea too.

So, is this what's happening?

  • The user has an account with a Secp256k1 key. They want to become a validator
  • The user creates a staking message cthat ontains this Secp256k1 key (hopefully enforced!) and an Ed25519 validator key
  • We wrap this message
  • The user attaches a BLS public key to the message
  • The user attaches a PoP to the message
  • The user signs the transaction with Secp256k1

And if so, would the following be a correct PoP:

PoP =  encrypt(key = BLS_sk, data = encrypt(key = Ed25519_sk, data = Secp256k1_pk))
msg = { BLS_pk, Ed25519_pk, PoP }
sig = sign(key = Secp256k1_sk, data = msg)
tx = { Secp256k1_pk, msg, sig }

check(tx) = 
  validate(key = tx.Secp256k1_pk, sig = tx.sig, data = tx.msg) &&
  tx.Secp256k1_pk == decrypt(key = tx.msg.Ed25519_pk, data = decrypt(key = tx.msg.BLS_pk, data = tx.msg.PoP))

Notably I replaced the innermost message with the user key, rather than the Ed25519 of the validator, so that another user cannot just take the PoP and try to register with the same keys, because they would not be able to produce a valid PoP without having access to the *_sk private keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this could work. Thanks! I'll use this version of PoP then and update the spec accordingly.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Great!

I'm generally confused about key handling in the Cosmos SDK. My naive assumption was that we can take the Ed25519 key from the transaction itself, and register the validator like that, but I'm not sure this is possible, or what is the actual mechanism to get the sender address.

If the transaction was not signed by the validator key, but some other account key, and the registration only included the Ed25519 public key, then we have to make sure that the Proof-of-Possession cannot be stolen by some other account. In that case, PoP would have to include three layers.

Sorry if I'm barking up the wrong tree.

My other remarks is that the staking message should be just sent to the epoching module for delaying, like @SebastianElvis is doing in his handlers.

Comment on lines 54 to 55
// RemoveBlsPubKey removes a BLS public key
func (rs RegistrationState) RemoveBlsPubKey(addr string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? As I understand you possibly wanted this to happen if the delayed transaction fails, but:

  • the epoching module doesn't know it has to do a callback with the results
  • what if it was some kind of a duplicate and the initial registration didn't fail, in which case we might remove the BLS key but leave the validator staked

Although your model of cleaning up after failed registrations is nice too. It would for sure need a callback mechanism. @SebastianElvis what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Adding this cleaning mechanism probably will not work since we don't know whether the staking message is executed successfully. We should allow the users to retry registration. One note is that a valid bls-sig should be from a validator that is staked (only registering BLS key is not enough).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well there's your 🐔 🥚 problem:

  • the validator will be registered by the delayed mechanism
  • a validator without a BLS key is useless to us, they can't sign a checkpoint
  • a BLS key without a registered validator just takes up space, but we can ignore it completely

So while it's not enough to have a BLS key, it's also not enough to have a registered validator without one, but while the former is benign, the latter will be able to propose Tendermint blocks.

Ideally we'd do the two together, but as discussed doing it up front allows us to reject at least some of it with a clear error message associated with the transaction, if there's a problem. You could check the current account balance for example, or even take it into a holding pool, if you are really fancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that we should keep this method because when a validator is removed by the staking module, we should remove the corresponding BLS key, too. The staking module has a hook called AfterValidatorRemoved. Maybe we should implement this hook by calling RemoveBlsPubKey()

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe they mean it as in the validator is removed from the active validator set, but they are not entirely forgotten, at least not by Tendermint: https://docs.tendermint.com/master/rpc/#/Info/validators

@gitferry
Copy link
Contributor Author

gitferry commented Jul 6, 2022

Thanks for the review @aakoshh. Please see the updates. Thanks!

@gitferry gitferry force-pushed the gai/checkpointing-registration branch from 9f021d4 to 44c44ff Compare July 6, 2022 04:39

// WrappedCreateValidator stores validator's BLS public key as well as corresponding MsgCreateValidator message
func (m msgServer) WrappedCreateValidator(goCtx context.Context, msg *types.MsgWrappedCreateValidator) (*types.MsgWrappedCreateValidatorResponse, error) {
// TODO: verify pop
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same as this

If the PoP was validated in ValidateBasic, would it have to be done here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very good question. It makes more sense that all the stateless verification is done in ValidateBasic.
ValidateBasic is called before the message is handed to the handler.

I'll worry about this in a future PR. Thanks for the reminder though.

Copy link
Contributor

@aakoshh aakoshh 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 looking very good now! 🙂

@gitferry
Copy link
Contributor Author

gitferry commented Jul 6, 2022

Thanks @aakoshh for the review! Please see the updates. Thanks!

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Good good 🎉

@gitferry gitferry merged commit 889f87c into main Jul 6, 2022
@gitferry gitferry deleted the gai/checkpointing-registration branch July 6, 2022 09:52
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.

3 participants