fix: match comets bls implmentation #22613
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes significant updates to the dependency management across various modules within the Cosmos SDK. Notably, the indirect dependency on Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
julienrbrt
added
the
backport/v0.52.x
tac0turtle
requested review from
JulianToledano and
raynaudoe
as code owners
|
@tac0turtle your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
crypto/keys/jsonkey.go (1)
Critical: Confirmed BLS public key size mismatch
The implementation has a critical issue with BLS key handling:
- The code uses CometBFT's BLS package name (
bls.PubKeyName) but implements key handling with Cosmos SDK'sbls12_381package- The
bls12_381package enforces a different key size throughbls12381.PubKeySizevalidation in theAddress()method- There's no validation for the 96-byte public key size from CometBFT in the current implementation
This mismatch will cause runtime panics when handling CometBFT's 96-byte BLS public keys, as confirmed by the code in
crypto/keys/bls12_381/key_cgo.go.🔗 Analysis chain
Line range hint
30-34: Critical: Potential public key size mismatch in BLS implementationThe current implementation might not properly handle the 96-byte public key size from CometBFT:
- The case statement uses Comet's BLS package name (
bls.PubKeyName)- But the implementation still uses Cosmos SDK's BLS type (
bls12_381.PubKey)This could lead to the same public key size issues mentioned in #22549 where CometBFT generates 96-byte keys but Cosmos SDK expects 48-byte keys.
Let's verify the public key size handling:
Let me gather more specific information about the BLS key size handling.
Let me check one more thing to verify the BLS package imports and constants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check BLS public key size handling across the codebase # Search for public key size validations echo "Searching for public key size validations..." rg --type go "len\(.*Key.*\)" # Search for BLS key type definitions echo "Searching for BLS key type definitions..." ast-grep --pattern 'type PubKey struct { Key []byte }'Length of output: 32611
Script:
#!/bin/bash # Search for BLS key size validations and related code echo "Searching for BLS key size validations..." rg -A 5 -B 5 "bls.*Key.*Size|bls.*Size.*Key" echo "Searching for BLS key imports and usage..." rg -A 2 "\"github.com/cometbft/cometbft/crypto/bls\"" echo "Searching for BLS key validation in Address method..." ast-grep --pattern 'func (pubKey PubKey) Address() $$$'Length of output: 3091
Script:
#!/bin/bash # Search for BLS package imports and constants echo "Searching for BLS package imports..." rg -l "github.com/cometbft/cometbft/crypto/bls" echo "Searching for BLS constants..." rg "const.*PubKeySize.*=.*96|const.*PubKeyName.*=.*"Length of output: 435
crypto/keys/multisig/codec.go (1)
Line range hint
1-33: Consider documenting the BLS implementation transitionThis change is part of a significant transition from Cosmos SDK's BLS implementation to CometBFT's implementation. To ensure a smooth transition:
- Consider adding a migration guide for users with existing BLS keys
- Document the key size differences (48 vs 96 bytes) and their implications
- Update relevant API documentation to reflect the new BLS implementation
- Consider adding version compatibility notes in the changelog
crypto/codec/amino.go (1)
Line range hint
1-31: Document the BLS implementation migration strategyGiven the significant changes in BLS implementation and the potential impact on existing systems:
- Consider adding detailed migration documentation
- Include examples of handling both old and new key formats
- Document the timeline for deprecating the old implementation
Would you like assistance in creating a migration guide or documentation updates?
crypto/codec/pubkey.go (1)
Line range hint
1-70: Consider implementing a clear migration strategyThe current implementation mixes both the old (
bls12_381) and new (bls12381) BLS implementations, which could lead to maintenance issues. Consider:
- Creating a migration plan to fully transition to Comet's BLS implementation
- Adding clear documentation about the transition period
- Implementing version checks to ensure compatibility during the migration
This will help prevent confusion and make the codebase more maintainable in the long term.
crypto/keys/bls12_381/key.go (1)
Line range hint
29-65: Enhance documentation for build configurationThe panic messages could be more informative, and we should add documentation explaining:
- When this stub implementation is used
- How to enable the actual BLS implementation
- The relationship with Comet's BLS package
Consider updating the panic messages and adding package documentation:
// NewPrivateKeyFromBytes build a new key from the given bytes. func NewPrivateKeyFromBytes(bz []byte) (PrivKey, error) { - panic("not implemented, build flags are required to use bls12_381 keys") + panic("BLS12-381 support is not enabled. Build with -tags=bls12381 to enable BLS support using Comet's implementation") }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (27)
client/v2/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/params/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
client/v2/go.mod(0 hunks)crypto/codec/amino.go(2 hunks)crypto/codec/pubkey.go(3 hunks)crypto/keys/bls12_381/const.go(0 hunks)crypto/keys/bls12_381/key.go(4 hunks)crypto/keys/bls12_381/key_cgo.go(7 hunks)crypto/keys/jsonkey.go(2 hunks)crypto/keys/multisig/codec.go(2 hunks)go.mod(0 hunks)server/v2/cometbft/go.mod(0 hunks)simapp/go.mod(0 hunks)simapp/v2/go.mod(0 hunks)tests/go.mod(0 hunks)x/accounts/defaults/base/go.mod(0 hunks)x/accounts/defaults/lockup/go.mod(0 hunks)x/accounts/defaults/multisig/go.mod(0 hunks)x/accounts/go.mod(0 hunks)x/authz/go.mod(0 hunks)x/bank/go.mod(0 hunks)x/circuit/go.mod(0 hunks)x/consensus/go.mod(0 hunks)x/distribution/go.mod(0 hunks)x/epochs/go.mod(0 hunks)x/evidence/go.mod(0 hunks)x/feegrant/go.mod(0 hunks)x/gov/go.mod(0 hunks)x/group/go.mod(0 hunks)x/mint/go.mod(0 hunks)x/nft/go.mod(0 hunks)x/params/go.mod(0 hunks)x/protocolpool/go.mod(0 hunks)x/slashing/go.mod(0 hunks)x/staking/go.mod(0 hunks)x/upgrade/go.mod(0 hunks)
💤 Files with no reviewable changes (28)
- client/v2/go.mod
- crypto/keys/bls12_381/const.go
- go.mod
- server/v2/cometbft/go.mod
- simapp/go.mod
- simapp/v2/go.mod
- tests/go.mod
- x/accounts/defaults/base/go.mod
- x/accounts/defaults/lockup/go.mod
- x/accounts/defaults/multisig/go.mod
- x/accounts/go.mod
- x/authz/go.mod
- x/bank/go.mod
- x/circuit/go.mod
- x/consensus/go.mod
- x/distribution/go.mod
- x/epochs/go.mod
- x/evidence/go.mod
- x/feegrant/go.mod
- x/gov/go.mod
- x/group/go.mod
- x/mint/go.mod
- x/nft/go.mod
- x/params/go.mod
- x/protocolpool/go.mod
- x/slashing/go.mod
- x/staking/go.mod
- x/upgrade/go.mod
🧰 Additional context used
📓 Path-based instructions (6)
crypto/codec/amino.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/codec/pubkey.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/bls12_381/key.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/bls12_381/key_cgo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/jsonkey.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/multisig/codec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
crypto/keys/jsonkey.go (1)
Line range hint 4-8: Verify the necessity of both BLS imports
The file imports both CometBFT's and Cosmos SDK's BLS packages. While this aligns with the transition to Comet's implementation, maintaining both imports might lead to confusion.
Let's check if both BLS packages are actually used:
✅ Verification successful
Let me gather more context about the specific usage in this file.
Both BLS imports are necessary and correctly used
The dual imports are intentional and serve different purposes:
- The CometBFT import (
bls) is used for the key type constant (bls.PubKeyName) - The Cosmos SDK import (
bls12_381) is used for the actual key implementation and address generation
This aligns with the PR's objective of transitioning to CometBFT's BLS implementation while maintaining compatibility with existing Cosmos SDK types. The imports are not redundant but complementary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of both BLS packages in the codebase
# Look for any cross-usage or potential conflicts
# Search for usage of both BLS packages
echo "Searching for usage of both BLS packages..."
rg --type go "bls12_381\."
rg --type go "github.com/cometbft/cometbft/crypto/bls12381"
Length of output: 1751
Script:
#!/bin/bash
# Check the specific usage of both BLS packages in jsonkey.go
echo "Content of jsonkey.go:"
cat crypto/keys/jsonkey.go
Length of output: 1053
crypto/keys/multisig/codec.go (2)
Line range hint 4-9: Verify the coexistence of both BLS implementations
The file now imports both the Comet BLS implementation (github.com/cometbft/cometbft/crypto/bls12381) and the Cosmos SDK BLS implementation (github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381). This could lead to confusion or potential runtime issues if both implementations are used inconsistently across the codebase.
Let's verify the usage of both BLS implementations across the codebase:
✅ Verification successful
Both BLS implementations are used intentionally for migration purposes
The coexistence of both BLS implementations is deliberate and properly handled:
- The old SDK BLS implementation (
bls12_381) is maintained for backward compatibility - The new CometBFT BLS implementation (
bls12381) is being introduced as the new standard - The codec registration in
crypto/codec/amino.goandcrypto/keys/multisig/codec.gocorrectly maps both implementations - The conversion logic between implementations is properly handled in
crypto/codec/pubkey.goandcrypto/codec/cmt.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for mixed usage of BLS implementations
# Look for files using both implementations which could lead to inconsistencies
echo "Files importing both BLS implementations:"
rg -l 'github\.com/cometbft/cometbft/crypto/bls12381' | xargs rg -l 'github\.com/cosmos/cosmos-sdk/crypto/keys/bls12_381'
echo -e "\nUsage of old BLS implementation:"
rg --type go 'bls12_381\.PubKey'
echo -e "\nUsage of new BLS implementation:"
rg --type go 'bls12381\.PubKey'
Length of output: 1476
30-30: Verify backward compatibility with existing BLS public keys
The registration is being updated to use bls12381.PubKeyName from CometBFT instead of the SDK's implementation. This change aligns with the PR objective to match Comet's BLS implementation, but we need to ensure:
- Existing BLS public keys in the system can still be deserialized
- The transition handles the different key sizes (96 bytes vs 48 bytes) correctly
Let's verify the impact:
crypto/codec/amino.go (1)
6-7: Verify the necessity of maintaining both BLS implementations
The code imports both the old (github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381) and new (github.com/cometbft/cometbft/crypto/bls12381) BLS implementations. While this might be intentional for migration purposes, it could lead to confusion and potential issues.
Let's check if both implementations are actually used:
✅ Verification successful
Both BLS implementations are actively used in a coordinated manner
The code analysis reveals a deliberate integration pattern where:
- The old implementation (
bls12_381) is used for type definitions and interfaces - The new implementation (
bls12381) provides the underlying cryptographic operations and constants
Key examples:
crypto/keys/bls12_381/key_cgo.gouses CometBFT's implementation for all cryptographic operationscrypto/codec/amino.goand other codec files use the old types for registration but with new constantsx/genutil/utils.godirectly uses CometBFT's implementation for key generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of both BLS implementations across the codebase
# Look for usage patterns of both implementations
echo "Checking usage of old BLS implementation:"
rg --type go "bls12_381\." -A 2
echo "Checking usage of new BLS implementation:"
rg --type go "bls12381\." -A 2
Length of output: 5379
crypto/codec/pubkey.go (1)
6-8: Verify the necessity of both BLS imports
The code imports both Comet's BLS implementation (bls12381) and Cosmos SDK's BLS implementation (bls12_381). According to the PR objectives, we're moving to Comet's implementation, so we should verify if both imports are necessary during the transition.
✅ Verification successful
Let me analyze the usage patterns to make a final determination.
Both BLS imports are necessary for the transition
The dual imports are actually required as part of the architecture:
github.com/cometbft/cometbft/crypto/bls12381is used for its constants and key type namesgit.luolix.top/cosmos/cosmos-sdk/crypto/keys/bls12_381provides the actual key implementations used in the conversion functions
The code shows a clear pattern where:
- CometBFT's BLS package provides the key type name (
bls12381.PubKeyName) - Cosmos SDK's BLS package provides the key structs (
bls12_381.PubKey)
This is consistent across both pubkey.go and cmt.go, making it a deliberate design choice for the transition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both BLS implementations are actually used
echo "Checking usage of both BLS implementations..."
rg --type go "bls12381\."
rg --type go "bls12_381\."
Length of output: 2454
Script:
#!/bin/bash
# Check the content of the specific files to understand the transition
echo "Checking crypto/codec/pubkey.go..."
cat crypto/codec/pubkey.go
echo -e "\nChecking crypto/codec/cmt.go..."
cat crypto/codec/cmt.go
Length of output: 5015
crypto/keys/bls12_381/key.go (3)
58-58: LGTM: Consistent key type alignment with Comet's implementation
The Type() method changes correctly align both private and public key types with Comet's BLS implementation.
Also applies to: 123-123
Line range hint 1-1: Verify the build configuration and implementation file
This file contains stub implementations that panic when BLS12-381 is not enabled. Given that we're switching to Comet's BLS implementation, we should verify:
- The existence of the actual implementation file (with
//go:build bls12381) - The build tags are correctly set up to use the right implementation
Also applies to: 11-11
✅ Verification successful
Build configuration is correctly set up
The build configuration is properly structured:
key.gohas//go:build !bls12381and contains stub implementations that panickey_cgo.gohas//go:build ((linux && amd64) || (linux && arm64) || (darwin && amd64) || (darwin && arm64) || (windows && amd64)) && bls12381and contains the actual implementation- The Comet BLS package is correctly imported and used across relevant files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for the corresponding implementation file and its build tags
fd -e go "key.*" crypto/keys/bls12_381/ --exec grep -l "//go:build" {} \;
# Verify if there are any other files importing the bls12381 package
rg "github.com/cometbft/cometbft/crypto/bls12381"
Length of output: 817
Script:
#!/bin/bash
# Let's check the build tags in both files
echo "=== key.go build tags ==="
head -n 5 crypto/keys/bls12_381/key.go
echo -e "\n=== key_cgo.go build tags ==="
head -n 5 crypto/keys/bls12_381/key_cgo.go
# Let's also check if key.go is indeed a stub implementation
echo -e "\n=== Checking for panic statements in key.go ==="
rg "panic\(" crypto/keys/bls12_381/key.go
# And verify the actual implementation in key_cgo.go
echo -e "\n=== Checking implementation in key_cgo.go ==="
ast-grep --pattern 'func (k *PrivKey) $$$'
Length of output: 1325
74-77: Verify key size compatibility with issue #22549
Given the reported issue about 96-byte vs 48-byte public key discrepancy (#22549), we should verify that bls.PrivKeySize matches the expected size requirements.
crypto/keys/bls12_381/key_cgo.go
Outdated
| @@ -74,24 +75,23 @@ func (privKey PrivKey) Equals(other cryptotypes.LedgerPrivKey) bool { | |||
|
|
|||
| // Type returns the type. | |||
| func (PrivKey) Type() string { | |||
| return KeyType | |||
| return bls.KeyType | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent use of package alias in Type() method.
The Type() method for PrivKey is returning bls.KeyType, but with the removal of the bls alias, this should be updated to use bls12381.KeyType to maintain consistency.
Apply this diff to fix the inconsistency:
func (PrivKey) Type() string {
- return bls.KeyType
+ return bls12381.KeyType
}Committable suggestion skipped: line range outside the PR's diff.
crypto/keys/bls12_381/key_cgo.go
Outdated
| bls "github.com/cometbft/cometbft/crypto/bls12381" | ||
| "github.com/cometbft/cometbft/crypto/tmhash" | ||
|
|
||
| bls12381 "github.com/cosmos/crypto/curves/bls12381" | ||
| bls12381 "github.com/cometbft/cometbft/crypto/bls12381" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate imports of the same package.
The package github.com/cometbft/cometbft/crypto/bls12381 is imported twice with different aliases (bls and bls12381). This can lead to confusion and is unnecessary. It's best to import the package once with a single alias to maintain clarity and consistency.
Apply this diff to fix the issue:
import (
"bytes"
"crypto/sha256"
"errors"
"fmt"
- bls "github.com/cometbft/cometbft/crypto/bls12381"
"github.com/cometbft/cometbft/crypto"
"github.com/cometbft/cometbft/crypto/tmhash"
- bls12381 "github.com/cometbft/cometbft/crypto/bls12381"
+ "github.com/cometbft/cometbft/crypto/bls12381"
"github.com/cosmos/cosmos-sdk/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
)Then, update all references of bls to bls12381 throughout the file to ensure consistency.
Committable suggestion skipped: line range outside the PR's diff.
| pk, _ := bls12381.NewPublicKeyFromBytes(pubKey.Key) | ||
| if len(pk.Bytes()) != bls12381.PubKeySize { | ||
| panic("pubkey is incorrect size") | ||
| } | ||
| return crypto.Address(tmhash.SumTruncated(pubKey.Key)) |
There was a problem hiding this comment.
Handle errors returned by NewPublicKeyFromBytes in Address() method.
In the Address() method, the error returned by bls12381.NewPublicKeyFromBytes(pubKey.Key) is ignored. Ignoring errors can lead to unexpected panics if the public key is invalid. It's important to check and handle the error to ensure robustness.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pk, _ := bls12381.NewPublicKeyFromBytes(pubKey.Key) | |
| if len(pk.Bytes()) != bls12381.PubKeySize { | |
| panic("pubkey is incorrect size") | |
| } | |
| return crypto.Address(tmhash.SumTruncated(pubKey.Key)) | |
| pk, err := bls12381.NewPublicKeyFromBytes(pubKey.Key) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid public key: %v", err)) | |
| } | |
| return crypto.Address(tmhash.SumTruncated(pubKey.Key)) |
| 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.
🛠️ 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 creating a migration layer that handles the key size differences
- Document the migration path for users
- Add validation checks for key compatibility
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)Committable suggestion skipped: line range outside the PR's diff.
| @@ -18,13 +19,13 @@ | |||
| ed25519.PubKeyName) | |||
| registrar.RegisterConcrete(&secp256k1.PubKey{}, | |||
| secp256k1.PubKeyName) | |||
| registrar.RegisterConcrete(&bls12_381.PubKey{}, bls12_381.PubKeyName) | |||
| registrar.RegisterConcrete(&bls12_381.PubKey{}, bls12381.PubKeyName) | |||
There was a problem hiding this comment.
💡 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 using bls12381.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:
- Line 22 imports CometBFT's
bls12381but registers SDK'sbls12_381.PubKey - The validation checks in
key_cgo.goconfirm different size requirements between implementations - The mismatch between type and name constant could cause serialization/deserialization issues
🔗 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:
#!/bin/bash
# Description: Check for key size validations and potential issues
# Look for size checks and panic conditions
echo "Checking for key size validations:"
rg --type go "public key must be \d+ bytes"
echo "Checking for BLS key size constants:"
rg --type go "const.*PubKeySize.*=.*\d+"
Length of output: 307
Script:
#!/bin/bash
# Let's check the actual BLS key size definitions and usages
echo "Checking BLS-related constants and types:"
rg --type go -A 5 "type.*PubKey.*struct"
echo "Checking BLS-related imports and package declarations:"
rg --type go "bls12381|bls12_381"
echo "Looking for key size validations in BLS implementations:"
rg --type go -A 5 "func.*ValidateKey|func.*Validate.*Key"
Length of output: 28920
Script:
#!/bin/bash
# Let's check for BLS-related constants and key size definitions
echo "Checking for BLS key size constants and validations:"
rg --type go -B 2 -A 2 "PubKeySize.*=.*\d+|SignatureLength.*=.*\d+|MaxMsgLen.*=.*\d+" crypto/keys/bls12_381/
echo "Checking BLS package imports and type definitions:"
rg --type go "package bls12381|package bls12_381" -A 10
echo "Looking for BLS key validation checks:"
rg --type go "len\(.*\).*!=" crypto/keys/bls12_381/
Length of output: 2488
Script:
#!/bin/bash
# Let's check the actual implementation and imports in the file under review
echo "Checking the content of the file under review:"
cat crypto/codec/amino.go
echo "Checking the imported bls12381 package constants:"
rg --type go -B 2 -A 2 "PubKeyName.*=|PrivKeyName.*=" crypto/keys/bls12_381/
Length of output: 1508
There was a problem hiding this comment.
I noticed the same, is it ok?
There was a problem hiding this comment.
@facundomedica, the mismatch between bls12_381.PubKey and bls12381.PubKeyName is not okay. Registering the SDK's bls12_381.PubKey with CometBFT's bls12381.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 and PubKeyName come from the same package. Here's how you can update the code:
registrar.RegisterConcrete(&bls12381.PubKey{}, bls12381.PubKeyName)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.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| case bls12381.PubKeyName: | ||
| return &bls12_381.PubKey{ | ||
| Key: pk.Value, | ||
| }, nil |
There was a problem hiding this comment.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case bls12381.PubKeyName: | |
| return &bls12_381.PubKey{ | |
| Key: pk.Value, | |
| }, nil | |
| 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 |
| return cryptokeys.JSONPubkey{ | ||
| KeyType: bls12_381.PubKeyName, | ||
| KeyType: bls12381.PubKeyName, | ||
| Value: pk.Bytes(), | ||
| }, nil |
There was a problem hiding this comment.
💡 Codebase verification
The BLS type naming inconsistency is a legitimate concern
Looking at the imports and usage in crypto/codec/pubkey.go, there is indeed an inconsistency:
- The type check uses
*bls12_381.PubKey(fromgit.luolix.top/cosmos/cosmos-sdk/crypto/keys/bls12_381) - The KeyType uses
bls12381.PubKeyName(fromgit.luolix.top/cometbft/cometbft/crypto/bls12381)
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 chain
Ensure consistent BLS type naming
There's an inconsistency between the type check (*bls12_381.PubKey) and the KeyType assignment (bls12381.PubKeyName). This mismatch could cause issues with type matching elsewhere in the system.
🏁 Scripts executed
The 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crypto/keys/bls12_381/key_cgo.go (2)
Line range hint
13-169: Successful transition to CometBFT's BLS implementation.The changes successfully implement the transition to CometBFT's BLS12-381 package while maintaining robust security checks and error handling. This aligns with the PR objective of fixing the BLS implementation to match Comet.
A few architectural benefits of this implementation:
- Consistent handling of large messages using SHA256 in both Sign and Verify operations
- Proper validation of key and signature sizes
- Clear error handling paths
Add test coverage for SHA256 fallback in BLS Sign method
The Sign method includes logic to handle messages larger than MaxMsgLen by using SHA256, but there are no tests covering this functionality. Please add test cases to verify:
- Signing messages smaller than MaxMsgLen (direct signing)
- Signing messages larger than MaxMsgLen (SHA256 fallback)
- Signature verification for both cases
🔗 Analysis chain
Line range hint
34-92: LGTM! Consistent implementation of BLS operations using CometBFT's package.The changes correctly implement private key operations using CometBFT's BLS12-381 package, with proper error handling and size checks.
Let's verify the test coverage for the SHA256 fallback in the Sign method:
Let me search specifically for BLS test files and check their content:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if we have tests covering the SHA256 fallback case rg -l "TestSign.*" --type go ast-grep --pattern 'func Test$_(t *testing.T) { $$$ msg := $$$ if len(msg) > bls12381.MaxMsgLen { $$$ } $$$ }'Length of output: 797
Script:
#!/bin/bash # Look for BLS-specific test files and their content fd "bls.*test.go" --type f rg -A 10 "func TestSign" crypto/keys/bls12_381/Length of output: 78
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
crypto/keys/bls12_381/key_cgo.go(7 hunks)scripts/build/build.mk(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
crypto/keys/bls12_381/key_cgo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
scripts/build/build.mk (1)
76-78: LGTM! The build tag update aligns with the BLS implementation changes.
The change from blst to bls12381 in the build configuration correctly reflects the transition to match Comet's BLS implementation.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Build tag transition from blst to bls12381 is correctly implemented
The verification confirms:
- Only one remaining reference to
blstin a comment withinbuild.mk - Consistent usage of
bls12381across the codebase:- Build configuration in
scripts/build/build.mk - BLS implementation imports from CometBFT
- Proper build tags in key implementation files
- Correct type registrations in codec files
- Build configuration in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old 'blst' build tag
# and verify the usage of the new 'bls12381' tag
# Search for any remaining references to the old 'blst' tag
echo "Checking for remaining 'blst' references:"
rg --type go --type make "blst"
# Search for usage of the new 'bls12381' tag
echo "Checking 'bls12381' usage:"
rg --type go --type make "bls12381"
Length of output: 2568
crypto/keys/bls12_381/key_cgo.go (1)
145-159: LGTM! Robust signature verification implementation.
The VerifySignature method includes important validations:
- Signature length check
- Public key validity check
- Consistent large message handling with SHA256
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
(cherry picked from commit 9d6d106) # Conflicts: # tests/integration/type_check.go # x/authz/go.mod # x/bank/go.mod # x/mint/go.mod # x/staking/go.mod
* main: feat(serverv2): add benchmarks of (old) cacheKV vs branch (#22497) fix: match comets bls implmentation (#22613) feat(math): Upstream GDA based decimal type (#21982) fix(client/v2/autocli): prevent duplicate addition of customCommands (#22576) build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580)
Description
This pr uses comet as a dependency for bls in order to make sure to match crypto design. We were using the cosmos/crypto pakg but the design became different between the two repos
closes #22549 & #22548
this can be a short term fix but it blocking meter.io currently
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
github.com/cosmos/crypto v0.1.2, to streamline the codebase and avoid potential vulnerabilities.Documentation
Chores