Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: define checkpointing registration proto and state #46
Changes from 3 commits
04cfab3
e7e8e51
d78df0f
c3d9e59
795bf98
44c44ff
00d3e90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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.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.
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 theMsgCreateValidator
that is wrapped in theMsgWrappedCreateValidator
, so I maybe can remove it from PoP.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.
That's a good idea too.
So, is this what's happening?
And if so, would the following be a correct 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.
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.
I believe this could work. Thanks! I'll use this version of PoP then and update the spec accordingly.
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.
Is this the same as this
If the PoP was validated in
ValidateBasic
, would it have to be done here as well?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 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.
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.
Do we need this? As I understand you possibly wanted this to happen if the delayed transaction fails, but:
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?
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.
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).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.
Well there's your 🐔 🥚 problem:
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.
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.
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. Thestaking
module has a hook calledAfterValidatorRemoved
. Maybe we should implement this hook by callingRemoveBlsPubKey()
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.
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