-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: match comets bls implmentation #22613
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
package codec | ||
|
||
import ( | ||
"github.com/cometbft/cometbft/crypto/bls12381" | ||
|
||
"cosmossdk.io/core/registry" | ||
|
||
bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381" | ||
|
@@ -18,13 +20,13 @@ func RegisterCrypto(registrar registry.AminoRegistrar) { | |
ed25519.PubKeyName) | ||
registrar.RegisterConcrete(&secp256k1.PubKey{}, | ||
secp256k1.PubKeyName) | ||
registrar.RegisterConcrete(&bls12_381.PubKey{}, bls12_381.PubKeyName) | ||
registrar.RegisterConcrete(&bls12_381.PubKey{}, bls12381.PubKeyName) | ||
registrar.RegisterConcrete(&kmultisig.LegacyAminoPubKey{}, | ||
kmultisig.PubKeyAminoRoute) | ||
registrar.RegisterInterface((*cryptotypes.PrivKey)(nil), nil) | ||
registrar.RegisterConcrete(&ed25519.PrivKey{}, | ||
ed25519.PrivKeyName) | ||
registrar.RegisterConcrete(&secp256k1.PrivKey{}, | ||
secp256k1.PrivKeyName) | ||
registrar.RegisterConcrete(&bls12_381.PrivKey{}, bls12_381.PrivKeyName) | ||
registrar.RegisterConcrete(&bls12_381.PrivKey{}, bls12381.PrivKeyName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure consistent migration of BLS key types Similar to the PubKey registration, there's a mismatch between the private key type and constant sources. To ensure a robust migration:
Consider this alternative approach: -registrar.RegisterConcrete(&bls12_381.PrivKey{}, bls12381.PrivKeyName)
+// TODO: Add migration layer
+type CompatBLSPrivKey struct {
+ *bls12381.PrivKey
+}
+registrar.RegisterConcrete(&CompatBLSPrivKey{}, bls12381.PrivKeyName)
|
||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,8 @@ | ||||||||||||||||||||||||
package codec | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||
"github.com/cometbft/cometbft/crypto/bls12381" | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
"cosmossdk.io/errors" | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
cryptokeys "github.com/cosmos/cosmos-sdk/crypto/keys" | ||||||||||||||||||||||||
|
@@ -29,7 +31,7 @@ func PubKeyToProto(pk cryptokeys.JSONPubkey) (cryptotypes.PubKey, error) { | |||||||||||||||||||||||
return &secp256k1.PubKey{ | ||||||||||||||||||||||||
Key: pk.Value, | ||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||
case bls12_381.PubKeyName: | ||||||||||||||||||||||||
case bls12381.PubKeyName: | ||||||||||||||||||||||||
return &bls12_381.PubKey{ | ||||||||||||||||||||||||
Key: pk.Value, | ||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||
Comment on lines
+34
to
37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for BLS public key size Given the known issue with BLS public key sizes (48 vs 96 bytes) mentioned in #22549, we should add validation here to ensure proper handling of different key sizes. Consider adding size validation: case bls12381.PubKeyName:
+ if len(pk.Value) != 96 {
+ return nil, errors.Wrapf(sdkerrors.ErrInvalidPubKey, "invalid bls12381 public key size: got %d, want 96", len(pk.Value))
+ }
return &bls12_381.PubKey{
Key: pk.Value,
}, nil 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
|
@@ -60,7 +62,7 @@ func PubKeyFromProto(pk cryptotypes.PubKey) (cryptokeys.JSONPubkey, error) { | |||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||
case *bls12_381.PubKey: | ||||||||||||||||||||||||
return cryptokeys.JSONPubkey{ | ||||||||||||||||||||||||
KeyType: bls12_381.PubKeyName, | ||||||||||||||||||||||||
KeyType: bls12381.PubKeyName, | ||||||||||||||||||||||||
Value: pk.Bytes(), | ||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||
Comment on lines
64
to
67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification The BLS type naming inconsistency is a legitimate concern Looking at the imports and usage in
This mismatch between SDK and CometBFT BLS packages could lead to type matching issues. The code should consistently use either the SDK's BLS implementation or CometBFT's implementation, but not mix them. 🔗 Analysis chainEnsure consistent BLS type naming There's an inconsistency between the type check ( 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for potential type matching issues
echo "Checking for KeyType usage..."
rg --type go "KeyType\s*[:=]\s*(bls12381|bls12_381)"
echo "Checking for PubKey type assertions..."
rg --type go "\.?\([*]?(bls12381|bls12_381)\.PubKey\)"
Length of output: 323 Script: #!/bin/bash
# Let's check the actual type definitions and imports to understand the naming pattern
echo "Checking BLS-related type definitions and imports..."
rg --type go -A 3 "type.*PubKey.*struct"
echo "Checking BLS-related constants and variables..."
rg --type go "(var|const).*bls"
echo "Checking import statements with BLS..."
rg --type go "^import.*bls"
# Check for any BLS-related string constants
echo "Checking BLS-related string constants..."
rg --type go "\"bls.*\""
Length of output: 15496 Script: #!/bin/bash
# Let's check the specific BLS package to understand the naming convention
echo "Checking BLS package contents..."
rg --type go -A 3 "package bls"
echo "Checking PubKeyName constant definition..."
rg --type go "PubKeyName.*=.*"
echo "Checking the actual import in the file under review..."
cat crypto/codec/pubkey.go
Length of output: 3355 |
||||||||||||||||||||||||
default: | ||||||||||||||||||||||||
|
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,8 +10,7 @@ import ( | |||||||||||||||||||||
|
||||||||||||||||||||||
"github.com/cometbft/cometbft/crypto" | ||||||||||||||||||||||
"github.com/cometbft/cometbft/crypto/tmhash" | ||||||||||||||||||||||
|
||||||||||||||||||||||
bls12381 "github.com/cosmos/crypto/curves/bls12381" | ||||||||||||||||||||||
"github.com/cometbft/cometbft/crypto/bls12381" | ||||||||||||||||||||||
|
||||||||||||||||||||||
"github.com/cosmos/cosmos-sdk/codec" | ||||||||||||||||||||||
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" | ||||||||||||||||||||||
|
@@ -32,20 +31,20 @@ var ( | |||||||||||||||||||||
|
||||||||||||||||||||||
// NewPrivateKeyFromBytes build a new key from the given bytes. | ||||||||||||||||||||||
func NewPrivateKeyFromBytes(bz []byte) (PrivKey, error) { | ||||||||||||||||||||||
secretKey, err := bls12381.SecretKeyFromBytes(bz) | ||||||||||||||||||||||
secretKey, err := bls12381.NewPrivateKeyFromBytes(bz) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return PrivKey{}, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
return PrivKey{ | ||||||||||||||||||||||
Key: secretKey.Marshal(), | ||||||||||||||||||||||
Key: secretKey.Bytes(), | ||||||||||||||||||||||
}, nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// GenPrivKey generates a new key. | ||||||||||||||||||||||
func GenPrivKey() (PrivKey, error) { | ||||||||||||||||||||||
secretKey, err := bls12381.RandKey() | ||||||||||||||||||||||
secretKey, err := bls12381.GenPrivKey() | ||||||||||||||||||||||
return PrivKey{ | ||||||||||||||||||||||
Key: secretKey.Marshal(), | ||||||||||||||||||||||
Key: secretKey.Bytes(), | ||||||||||||||||||||||
}, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -57,13 +56,13 @@ func (privKey PrivKey) Bytes() []byte { | |||||||||||||||||||||
// PubKey returns the private key's public key. If the privkey is not valid | ||||||||||||||||||||||
// it returns a nil value. | ||||||||||||||||||||||
func (privKey PrivKey) PubKey() cryptotypes.PubKey { | ||||||||||||||||||||||
secretKey, err := bls12381.SecretKeyFromBytes(privKey.Key) | ||||||||||||||||||||||
secretKey, err := bls12381.NewPrivateKeyFromBytes(privKey.Key) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
return &PubKey{ | ||||||||||||||||||||||
Key: secretKey.PublicKey().Marshal(), | ||||||||||||||||||||||
Key: secretKey.PubKey().Bytes(), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -74,24 +73,23 @@ func (privKey PrivKey) Equals(other cryptotypes.LedgerPrivKey) bool { | |||||||||||||||||||||
|
||||||||||||||||||||||
// Type returns the type. | ||||||||||||||||||||||
func (PrivKey) Type() string { | ||||||||||||||||||||||
return KeyType | ||||||||||||||||||||||
return bls12381.KeyType | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Sign signs the given byte array. If msg is larger than | ||||||||||||||||||||||
// MaxMsgLen, SHA256 sum will be signed instead of the raw bytes. | ||||||||||||||||||||||
func (privKey PrivKey) Sign(msg []byte) ([]byte, error) { | ||||||||||||||||||||||
secretKey, err := bls12381.SecretKeyFromBytes(privKey.Key) | ||||||||||||||||||||||
secretKey, err := bls12381.NewPrivateKeyFromBytes(privKey.Key) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if len(msg) > MaxMsgLen { | ||||||||||||||||||||||
if len(msg) > bls12381.MaxMsgLen { | ||||||||||||||||||||||
hash := sha256.Sum256(msg) | ||||||||||||||||||||||
sig := secretKey.Sign(hash[:]) | ||||||||||||||||||||||
return sig.Marshal(), nil | ||||||||||||||||||||||
return secretKey.Sign(hash[:]) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
sig := secretKey.Sign(msg) | ||||||||||||||||||||||
return sig.Marshal(), nil | ||||||||||||||||||||||
|
||||||||||||||||||||||
return secretKey.Sign(msg) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// MarshalAmino overrides Amino binary marshaling. | ||||||||||||||||||||||
|
@@ -101,7 +99,7 @@ func (privKey PrivKey) MarshalAmino() ([]byte, error) { | |||||||||||||||||||||
|
||||||||||||||||||||||
// UnmarshalAmino overrides Amino binary marshaling. | ||||||||||||||||||||||
func (privKey *PrivKey) UnmarshalAmino(bz []byte) error { | ||||||||||||||||||||||
if len(bz) != PrivKeySize { | ||||||||||||||||||||||
if len(bz) != bls12381.PrivKeySize { | ||||||||||||||||||||||
return errors.New("invalid privkey size") | ||||||||||||||||||||||
} | ||||||||||||||||||||||
privKey.Key = bz | ||||||||||||||||||||||
|
@@ -135,35 +133,30 @@ var _ cryptotypes.PubKey = &PubKey{} | |||||||||||||||||||||
// | ||||||||||||||||||||||
// The function will panic if the public key is invalid. | ||||||||||||||||||||||
func (pubKey PubKey) Address() crypto.Address { | ||||||||||||||||||||||
pk, _ := bls12381.PublicKeyFromBytes(pubKey.Key) | ||||||||||||||||||||||
if len(pk.Marshal()) != PubKeySize { | ||||||||||||||||||||||
pk, _ := bls12381.NewPublicKeyFromBytes(pubKey.Key) | ||||||||||||||||||||||
if len(pk.Bytes()) != bls12381.PubKeySize { | ||||||||||||||||||||||
panic("pubkey is incorrect size") | ||||||||||||||||||||||
} | ||||||||||||||||||||||
return crypto.Address(tmhash.SumTruncated(pubKey.Key)) | ||||||||||||||||||||||
Comment on lines
+136
to
140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle errors returned by In the Apply this diff to handle the error: func (pubKey PubKey) Address() crypto.Address {
- pk, _ := bls12381.NewPublicKeyFromBytes(pubKey.Key)
- if len(pk.Bytes()) != bls12381.PubKeySize {
- panic("pubkey is incorrect size")
- }
+ pk, err := bls12381.NewPublicKeyFromBytes(pubKey.Key)
+ if err != nil {
+ panic(fmt.Sprintf("invalid public key: %v", err))
+ }
return crypto.Address(tmhash.SumTruncated(pubKey.Key))
} Alternatively, consider redesigning the method to return an error instead of panicking, if that aligns with the overall design of the codebase. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// VerifySignature verifies the given signature. | ||||||||||||||||||||||
func (pubKey PubKey) VerifySignature(msg, sig []byte) bool { | ||||||||||||||||||||||
if len(sig) != SignatureLength { | ||||||||||||||||||||||
if len(sig) != bls12381.SignatureLength { | ||||||||||||||||||||||
return false | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
pubK, err := bls12381.PublicKeyFromBytes(pubKey.Key) | ||||||||||||||||||||||
pubK, err := bls12381.NewPublicKeyFromBytes(pubKey.Key) | ||||||||||||||||||||||
if err != nil { // invalid pubkey | ||||||||||||||||||||||
return false | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if len(msg) > MaxMsgLen { | ||||||||||||||||||||||
if len(msg) > bls12381.MaxMsgLen { | ||||||||||||||||||||||
hash := sha256.Sum256(msg) | ||||||||||||||||||||||
msg = hash[:] | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
ok, err := bls12381.VerifySignature(sig, [MaxMsgLen]byte(msg[:MaxMsgLen]), pubK) | ||||||||||||||||||||||
if err != nil { // bad signature | ||||||||||||||||||||||
return false | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
return ok | ||||||||||||||||||||||
return pubK.VerifySignature(msg, sig) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Bytes returns the byte format. | ||||||||||||||||||||||
|
@@ -173,7 +166,7 @@ func (pubKey PubKey) Bytes() []byte { | |||||||||||||||||||||
|
||||||||||||||||||||||
// Type returns the key's type. | ||||||||||||||||||||||
func (PubKey) Type() string { | ||||||||||||||||||||||
return KeyType | ||||||||||||||||||||||
return bls12381.KeyType | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Equals returns true if the other's type is the same and their bytes are deeply equal. | ||||||||||||||||||||||
|
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.
💡 Codebase verification
Critical: Incorrect BLS key registration detected
The review comment is correct. The code is registering
bls12_381.PubKey
from the SDK package but usingbls12381.PubKeyName
from CometBFT's package, which is a mismatch. This could lead to runtime issues due to different key sizes and implementations between the two packages.Key findings from the code:
bls12381
but registers SDK'sbls12_381.PubKey
key_cgo.go
confirm different size requirements between implementations🔗 Analysis chain
Critical: Potential key size mismatch in PubKey registration
There's a concerning mismatch where we're registering the old BLS type (
bls12_381.PubKey
) but using the new constant (bls12381.PubKeyName
). According to issue #22549, there's a fundamental difference in key sizes (96 bytes vs 48 bytes) between implementations.Let's verify the key size handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 307
Script:
Length of output: 28920
Script:
Length of output: 2488
Script:
Length of output: 1508
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 noticed the same, is it ok?
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.
@facundomedica, the mismatch between
bls12_381.PubKey
andbls12381.PubKeyName
is not okay. Registering the SDK'sbls12_381.PubKey
with CometBFT'sbls12381.PubKeyName
can lead to runtime issues due to differences in key sizes and implementations between the two packages.To resolve this issue, you should ensure that both the
PubKey
andPubKeyName
come from the same package. Here's how you can update the code:By using
bls12381.PubKey
from the CometBFT package, you ensure consistency between the key type and the key name, preventing potential serialization and deserialization issues.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 keys are the same in both repos. Didn't want to duplicate and open a window for the mismatch to occur again
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.