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: add bls verification #239

Merged
merged 7 commits into from
Jul 8, 2023
Merged

Conversation

j75689
Copy link
Collaborator

@j75689 j75689 commented Jul 6, 2023

Description

add signature verification for BLS key when creating, editing the validator.

Rationale

add a Proof of Possession to ensure the validator holds the BLS key.

Example

add an example CLI or API response...

Changes

Notable changes:

  • staking

@@ -628,3 +656,23 @@ func (ms msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdatePara

return &types.MsgUpdateParamsResponse{}, nil
}

// CheckBlsProof checks the BLS signature of the validator
func (ms msgServer) CheckBlsProof(goCtx context.Context, blsPk, sig []byte, valAddr sdk.Address) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not return error only, error means failed to verify.

Copy link
Collaborator

Choose a reason for hiding this comment

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

goctx and valAddr is useless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

// SignMsgKeysCmd returns the Cobra Command for signing messages with the private key of a given name.
func SignMsgKeysCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "sign [message]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a dangerous action, please note user to take care.

And give an example, it can be used to do possesion of proof..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -93,6 +93,10 @@ func (pubKey *PubKey) VerifySignature(msg, sig []byte) bool {
sig = sig[:len(sig)-1]
}

if len(msg) != crypto.DigestLength {
Copy link
Collaborator

@unclezoro unclezoro Jul 7, 2023

Choose a reason for hiding this comment

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

why we add this? I did not expect any changes of ethsecp256k1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because the ethsecp256k1 Sign method allows the user to pass the message in 32bytes and does not force the message to be hashed.

However, hashing of the message is enforced when verifying the signature.

The logic on both sides is inconsistent.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyways, I would remove these changes first to avoid causing other errors.

@unclezoro unclezoro merged commit 2dd8c5b into bnb-chain:develop Jul 8, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants