minimal bls-into-sdk #20294
minimal bls-into-sdk #20294
Conversation
WalkthroughWalkthroughThe recent updates focus on integrating BLS12-381 cryptographic support across various components of the Cosmos SDK. This includes modifications in key management, codec registration, and command line interfaces to handle the new key type. Changes ensure that the new cryptographic algorithm is supported throughout the system, from key generation to node initialization, enhancing the flexibility and security of the consensus mechanisms. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
crypto/keys/bls12_381/keys.pb.gois excluded by!**/*.pb.go
Files selected for processing (12)
- Makefile (1 hunks)
- crypto/codec/amino.go (3 hunks)
- crypto/codec/cmt.go (3 hunks)
- crypto/codec/proto.go (2 hunks)
- crypto/hd/algo.go (1 hunks)
- crypto/keys/bls12_381/keys.go (1 hunks)
- crypto/keys/multisig/codec.go (2 hunks)
- x/genutil/client/cli/collect.go (2 hunks)
- x/genutil/client/cli/gentx.go (2 hunks)
- x/genutil/client/cli/init.go (4 hunks)
- x/genutil/utils.go (3 hunks)
- x/genutil/utils_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- Makefile
Additional Context Used
GitHub Check Runs (2)
split-test-files failure (3)
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 43-43:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/params; to add it:dependency-review failure (16)
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/blst; to add it:
crypto/keys/bls12_381/keys.go: [failure] 43-43:
no required module provides package github.com/itsdevbear/comet-bls12-381/bls/params; to add it:
crypto/keys/bls12_381/keys.go: [failure] 42-42:
could not import github.com/itsdevbear/comet-bls12-381/bls/blst (invalid package name: "")
crypto/keys/bls12_381/keys.go: [failure] 43-43:
could not import github.com/itsdevbear/comet-bls12-381/bls/params (invalid package name: "")
Path-based Instructions (11)
crypto/keys/multisig/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/codec/proto.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/codec/amino.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"crypto/hd/algo.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/client/cli/collect.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/codec/cmt.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/client/cli/init.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.crypto/keys/bls12_381/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/client/cli/gentx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (17)
crypto/keys/multisig/codec.go (2)
7-7: Ensure that the new importbls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is used elsewhere in the file.Verification successful
The verification process has confirmed that the new import
bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is indeed utilized within thecodec.gofile. Specifically, thebls12_381.PubKeyis registered withAminoCdc, which indicates that the import is actively used beyond just being declared. This aligns with the expectations set in the review comment.
- The import is used to register
bls12_381.PubKeywithAminoCdcunder the namebls12_381.PubKeyName.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new import is used in the file. # Test: Search for the usage of `bls12_381` in the file. Expect: At least one usage apart from the import statement. rg --type go 'bls12_381' crypto/keys/multisig/codec.go | grep -v 'import'Length of output: 212
31-32: Registration ofbls12_381.PubKeyis correctly added. Ensure that all necessary tests are updated to cover this new type.crypto/codec/proto.go (3)
5-5: Ensure that the new importbls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is used elsewhere in the file.Verification successful
The verification process has confirmed that the new import
bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is indeed utilized within theproto.gofile. Specifically, the import is used to register implementations for both public and private keys related to BLS12-381. This aligns with the intended modifications mentioned in the review comment.
- The
bls12_381.PubKeyis registered with the registry for public keys.- The
bls12_381.PrivKeyis registered with the registry for private keys.These findings confirm that the import is not only present but is actively used in the context of the file, fulfilling the purpose of its inclusion.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new import is used in the file. # Test: Search for the usage of `bls12_381` in the file. Expect: At least one usage apart from the import statement. rg --type go 'bls12_381' crypto/codec/proto.go | grep -v 'import'Length of output: 248
19-19: Registration ofbls12_381.PubKeyis correctly added. Ensure that all necessary tests are updated to cover this new type.
26-26: Registration ofbls12_381.PrivKeyis correctly added. Ensure that all necessary tests are updated to cover this new type.crypto/codec/amino.go (3)
7-7: Ensure that the new importbls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is used elsewhere in the file.Verification successful
The verification process has confirmed that the new import
bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"is indeed utilized within theamino.gofile. Specifically, the import is used to register both the public and private key types associated with BLS12-381 in the codec, as evidenced by the lines:
cdc.RegisterConcrete(&bls12_381.PubKey{}, bls12_381.PubKeyName, nil)cdc.RegisterConcrete(&bls12_381.PrivKey{}, bls12_381.PrivKeyName, nil)These lines indicate that the import is not only present but actively used to facilitate the functionality related to BLS12-381 keys. Therefore, the initial review comment has been successfully verified.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new import is used in the file. # Test: Search for the usage of `bls12_381` in the file. Expect: At least one usage apart from the import statement. rg --type go 'bls12_381' crypto/codec/amino.go | grep -v 'import'Length of output: 269
24-24: Registration ofbls12_381.PubKeyis correctly added. Ensure that all necessary tests are updated to cover this new type.
35-35: Registration ofbls12_381.PrivKeyis correctly added. Ensure that all necessary tests are updated to cover this new type.x/genutil/utils_test.go (1)
56-56: The test forInitializeNodeValidatorFilesFromMnemonichas been updated to include theed25519key type. Ensure that tests for other key types, especially the newly introducedbls12_381, are also included.x/genutil/client/cli/collect.go (1)
31-36: The new flagFlagConsensusKeyAlgois correctly added and used to determine the consensus key algorithm. Ensure that error handling and user feedback are adequate when an invalid key type is specified.crypto/codec/cmt.go (2)
27-30: The conversion from CMT'scmtprotocrypto.PublicKey_Bls12381tobls12_381.PubKeyis correctly implemented. Ensure that all necessary tests are updated to cover this new conversion.
51-56: The conversion to CMT'scmtprotocrypto.PublicKey_Bls12381frombls12_381.PubKeyis correctly implemented. Ensure that all necessary tests are updated to cover this new conversion.x/genutil/utils.go (1)
Line range hint
49-107: The functionsInitializeNodeValidatorFilesandInitializeNodeValidatorFilesFromMnemonichave been updated to handle thebls12_381key type. Ensure that error handling is robust, especially for the case where BLS keys do not support mnemonics yet.Verification successful
The review comment regarding the error handling for BLS keys with mnemonics in the
InitializeNodeValidatorFilesFromMnemonicfunction is accurate. The code snippet provided explicitly handles the scenario where BLS keys do not support mnemonics, which aligns with the expectations set in the review comment. Here is the relevant part of the code:case "bls12_381": // TODO: need to add support for getting from mnemonic in Comet. return "", nil, fmt.Errorf("BLS key type does not support mnemonic")This error handling is robust and directly addresses the case mentioned. Therefore, the review comment is consistent with the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify robust error handling for BLS keys with mnemonics. # Test: Search for error handling related to BLS keys and mnemonics. Expect: At least one error handling case. rg --type go 'bls12_381' --glob '*.go' | grep 'error'Length of output: 51
x/genutil/client/cli/init.go (1)
116-121: Successfully integrated the newFlagConsensusKeyAlgofor specifying the consensus key algorithm. Ensure that this flag is consistently handled across all relevant parts of the application.Verification successful
The new flag
FlagConsensusKeyAlgois consistently handled across all relevant parts of the application, as evidenced by its usage in multiple CLI-related files (collect.go,gentx.go, andinit.go). Each usage involves setting a default value and retrieving the flag's value in a similar manner, which aligns with the intended integration of the BLS12-381 cryptographic scheme.
x/genutil/client/cli/collect.gox/genutil/client/cli/gentx.gox/genutil/client/cli/init.goScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new flag `FlagConsensusKeyAlgo` is handled correctly in all relevant parts of the application. # Test: Search for the flag usage. Expect: Consistent handling across the application. rg --type go $'FlagConsensusKeyAlgo'Length of output: 894
crypto/keys/bls12_381/keys.go (2)
47-58: Constants for key sizes and type assertions are correctly implemented and conform to the BLS12-381 specifications.
77-142: Key generation and serialization methods are correctly implemented using theblstlibrary, with proper error handling and adherence to the BLS12-381 specifications.x/genutil/client/cli/gentx.go (1)
66-71: Successfully integrated the newFlagConsensusKeyAlgofor specifying the consensus key algorithm in theGenTxCmdfunction. Ensure that this flag is consistently handled across all relevant parts of the application.Verification successful
The review of the new flag
FlagConsensusKeyAlgoacross the application indicates consistent and correct implementation in all relevant parts. The flag is uniformly declared and used within thex/genutil/client/clidirectory, ensuring coherent functionality across different commands.
- Files and usage:
init.go: Declaration and retrieval of the flag value.gentx.go: Declaration and retrieval of the flag value.collect.go: Declaration and retrieval of the flag value.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new flag `FlagConsensusKeyAlgo` is handled correctly in all relevant parts of the application. # Test: Search for the flag usage. Expect: Consistent handling across the application. rg --type go $'FlagConsensusKeyAlgo'Length of output: 894
| "github.com/itsdevbear/comet-bls12-381/bls/blst" | ||
| "github.com/itsdevbear/comet-bls12-381/bls/params" |
There was a problem hiding this comment.
The packages github.com/itsdevbear/comet-bls12-381/bls/blst and github.com/itsdevbear/comet-bls12-381/bls/params are not found. Ensure these packages are correctly included in the module dependencies.
+ require (
+ github.com/itsdevbear/comet-bls12-381 v0.1.0
+ )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.
| "github.com/itsdevbear/comet-bls12-381/bls/blst" | |
| "github.com/itsdevbear/comet-bls12-381/bls/params" | |
| "github.com/itsdevbear/comet-bls12-381/bls/blst" | |
| "github.com/itsdevbear/comet-bls12-381/bls/params" | |
| require ( | |
| github.com/itsdevbear/comet-bls12-381 v0.1.0 | |
| ) |
| // Ed25519Type represents the Ed25519Type signature system. | ||
| // It is currently not supported for end-user keys (wallets/ledgers). | ||
| Bls12_381Type = PubKeyType("bls12_381") |
There was a problem hiding this comment.
Tip
Codebase Verification
The constant Bls12_381Type is declared in algo.go but is not currently used in any cryptographic operations within the codebase. This suggests that the integration of this new key type into cryptographic operations might not be complete, or it is possibly planned for future development.
crypto/hd/algo.go: Declaration ofBls12_381Typewithout usage in cryptographic operations.
Analysis chain
The constant Bls12_381Type has been correctly added to represent the BLS12-381 key type. Ensure that this new type is handled appropriately in all relevant cryptographic operations.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `Bls12_381Type` is handled in cryptographic operations.
# Test: Search for the usage of `Bls12_381Type` in cryptographic operations. Expect: At least one usage.
rg --type go 'Bls12_381Type' --glob '*.go'
Length of output: 101
| @@ -57,7 +62,7 @@ func CollectGenTxsCmd(genBalIterator types.GenesisBalancesIterator, validator ty | |||
| return displayInfo(toPrint) | |||
| }, | |||
| } | |||
|
|
|||
| cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key") | |||
There was a problem hiding this comment.
The definition of the FlagConsensusKeyAlgo flag with a default value of ed25519 is appropriate. Consider adding a description or help text that lists all supported key types to improve user experience.
- cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key")
+ cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key (supported: ed25519, bls12_381)")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.
| cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key") | |
| cmd.Flags().String(FlagConsensusKeyAlgo, "ed25519", "algorithm to use for the consensus key (supported: ed25519, bls12_381)") |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func (pubKey *PubKey) Address() crypto.Address { | ||
| pk, _ := blst.PublicKeyFromBytes(pubKey.Key) | ||
| if len(pk.Marshal()) != PubKeySize { | ||
| panic("pubkey is incorrect size") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
|
Description
Closes: #XXXX
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.
I have...
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes