feat: port bls12381 from v0.52.rc2 #24872
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for BLS12-381 cryptographic keys throughout the codebase. This includes adding BLS12-381 key types to key generation, encoding/decoding, interface registration, and multisig handling. The node validator initialization functions and related CLI commands are updated to accept a key type parameter, enabling explicit selection of consensus key algorithms during initialization and testing. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/User
participant CLI_Command
participant GenUtil
participant CryptoKeys
CLI/User->>CLI_Command: Run collect/gentx/init command (with --consensus-key-algo)
CLI_Command->>GenUtil: InitializeNodeValidatorFiles(config, keyType)
alt keyType == "ed25519"
GenUtil->>CryptoKeys: Generate Ed25519 key
else keyType == "bls12_381"
GenUtil->>CryptoKeys: Generate BLS12-381 key
end
CryptoKeys-->>GenUtil: Return PrivKey, PubKey
GenUtil-->>CLI_Command: Return nodeID, PubKey
CLI_Command-->>CLI/User: Output result
sequenceDiagram
participant Codec
participant Registry
participant BLS12_381Key
Codec->>Registry: Register BLS12-381 PubKey/PrivKey types
Registry->>BLS12_381Key: Recognize BLS12-381 key types for encoding/decoding
BLS12_381Key-->>Registry: Provide implementation for cryptotypes interfaces
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration 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: 7
♻️ Duplicate comments (1)
x/genutil/utils.go (1)
101-109: 🛠️ Refactor suggestionMnemonic path exhibits same silent fallback
The
default:branch in the mnemonic section again reverts to ed25519. Mirror the validation above to keep behaviour consistent.
🧹 Nitpick comments (8)
crypto/hd/algo.go (1)
21-24: Constant name & comment style are inconsistent with existing onesAll previous constants use CamelCase without an underscore in the identifier (e.g.
Secp256k1Type,Ed25519Type).
Bls12_381Typeintroduces an underscore mid-identifier, diverging from the convention and forcing
call-sites to contain mixed styles.Recommend:
-// Bls12_381Type represents the Bls12_381Type signature system. -// It is currently not supported for end-user keys (wallets/ledgers). -Bls12_381Type = PubKeyType("bls12_381") +// Bls12381Type represents the BLS12-381 signature system. +// It is currently not supported for end-user keys (wallets/ledgers). +Bls12381Type = PubKeyType("bls12_381")This keeps the public identifier style coherent and fixes the duplicate wording in the comment.
crypto/codec/amino.go (1)
29-30: BLSPubKeyis being registered inside the PrivKey sectionAlthough Amino does not technically forbid registering a concrete type after the corresponding interface block, placing the
PubKeyregistration after thePrivKeyblock is misleading and easy to miss during future maintenance. Re-group it with the otherPubKeyregistrations for clarity.@@ cdc.RegisterConcrete(&kmultisig.LegacyAminoPubKey{}, kmultisig.PubKeyAminoRoute, nil) + + // --- PubKey concrete types --- + cdc.RegisterConcrete(&bls12_381.PubKey{}, bls12381.PubKeyName, nil) cdc.RegisterInterface((*cryptotypes.PrivKey)(nil), nil) cdc.RegisterConcrete(&ed25519.PrivKey{}, ed25519.PrivKeyName, nil) cdc.RegisterConcrete(&secp256k1.PrivKey{}, secp256k1.PrivKeyName, nil) - cdc.RegisterConcrete(&bls12_381.PubKey{}, bls12381.PubKeyName, nil) + + // --- PrivKey concrete types --- + cdc.RegisterConcrete(&bls12_381.PrivKey{}, bls12381.PrivKeyName, nil)x/genutil/client/cli/init_test.go (1)
240-244: Prefer symbolic constant for the key typeHard-coding
"ed25519"duplicates knowledge that already exists in the codebase and may diverge in the future (e.g. a constant is renamed). Import and use the key-type constant instead (e.g.ed25519.KeyTypeorhd.Ed25519Type, depending on where it lives).crypto/codec/cmt.go (1)
52-57: Consistency with KeyType constantWhen converting to Tendermint, you correctly map
bls12_381.PubKey.
Consider also gating this case onpk.Type() == bls12381.KeyTypeto future-proof the switch if other BLS implementations are introduced.x/genutil/utils.go (1)
49-57: Out-of-date comment and public signatureBoth comments still state “Key type is ed25519”, which is now misleading. Update the comment header so future readers realise the function is polymorphic.
-// InitializeNodeValidatorFiles creates private validator and p2p configuration files. Key type is ed25519. +// InitializeNodeValidatorFiles creates private validator and p2p configuration files. +// The keyType argument selects the consensus key algorithm (e.g. "ed25519", "bls12_381").crypto/keys/bls12_381/key.go (1)
30-65: Stub implementation should return typed errors, not panicBuild-tag stubs are great, but panicking inside every method makes accidental usage fatal at runtime. Consider returning a descriptive
ErrBLSDisabledinstead – this keeps the binary usable even when a higher-level call path forgets to gate its execution.-func (privKey PrivKey) Sign(msg []byte) ([]byte, error) { - panic("not implemented, build flags are required to use bls12_381 keys") -} +var ErrBLSDisabled = errors.New("bls12_381 build tag not enabled") + +func (privKey PrivKey) Sign(msg []byte) ([]byte, error) { + return nil, ErrBLSDisabled +}Same approach applies to the other panic methods.
crypto/keys/bls12_381/key_cgo.go (2)
31-40: Missing size validation inNewPrivateKeyFromBytesIf
bzis the wrong length,bls12381.NewPrivateKeyFromBytesalready errors, but we still wrap the returned bytes without re-checking. A malformed key therefore propagates unvalidated into the SDK.secretKey, err := bls12381.NewPrivateKeyFromBytes(bz) if err != nil { return PrivKey{}, err } -return PrivKey{Key: secretKey.Bytes()}, nil +raw := secretKey.Bytes() +if len(raw) != bls12381.PrivKeySize { + return PrivKey{}, fmt.Errorf("invalid privkey size %d", len(raw)) +} +return PrivKey{Key: raw}, nil
128-135:Address()ignores parsing error – could panic with invalid bytesUse the error returned by
NewPublicKeyFromBytesand fail fast with a meaningful panic or error.-pk, _ := bls12381.NewPublicKeyFromBytes(pubKey.Key) -if len(pk.Bytes()) != bls12381.PubKeySize { +pk, err := bls12381.NewPublicKeyFromBytes(pubKey.Key) +if err != nil { + panic(fmt.Sprintf("invalid BLS12-381 public key: %v", err)) +} +if len(pk.Bytes()) != bls12381.PubKeySize {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
crypto/keys/bls12_381/keys.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (14)
crypto/codec/amino.go(2 hunks)crypto/codec/cmt.go(3 hunks)crypto/codec/proto.go(2 hunks)crypto/hd/algo.go(1 hunks)crypto/keys/bls12_381/key.go(1 hunks)crypto/keys/bls12_381/key_cgo.go(1 hunks)crypto/keys/multisig/codec.go(2 hunks)simapp/simd/cmd/testnet.go(1 hunks)testutil/network/network.go(2 hunks)x/genutil/client/cli/collect.go(1 hunks)x/genutil/client/cli/gentx.go(1 hunks)x/genutil/client/cli/init.go(1 hunks)x/genutil/client/cli/init_test.go(1 hunks)x/genutil/utils.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
x/genutil/client/cli/init_test.go (1)
x/genutil/utils.go (1)
InitializeNodeValidatorFiles(50-52)
testutil/network/network.go (2)
x/genutil/utils.go (1)
InitializeNodeValidatorFilesFromMnemonic(56-125)crypto/keys/ed25519/ed25519.go (1)
PrivKeyName(23-23)
crypto/codec/amino.go (3)
crypto/keys/bls12_381/key.go (2)
PubKey(122-124)PrivKey(57-59)crypto/keys/bls12_381/key_cgo.go (2)
PubKey(157-159)PrivKey(74-76)crypto/keys/ed25519/ed25519.go (2)
PubKeyName(24-24)PrivKeyName(23-23)
simapp/simd/cmd/testnet.go (1)
x/genutil/utils.go (1)
InitializeNodeValidatorFiles(50-52)
crypto/keys/multisig/codec.go (3)
crypto/keys/bls12_381/key.go (1)
PubKey(122-124)crypto/keys/bls12_381/key_cgo.go (1)
PubKey(157-159)crypto/keys/ed25519/ed25519.go (1)
PubKeyName(24-24)
x/genutil/client/cli/init.go (1)
x/genutil/utils.go (1)
InitializeNodeValidatorFilesFromMnemonic(56-125)
x/genutil/client/cli/collect.go (3)
x/genutil/client/cli/init.go (1)
FlagConsensusKeyAlgo(42-42)errors/errors.go (1)
Wrap(190-206)x/genutil/utils.go (1)
InitializeNodeValidatorFiles(50-52)
crypto/codec/cmt.go (2)
crypto/keys/bls12_381/key.go (1)
PubKey(122-124)crypto/keys/bls12_381/key_cgo.go (1)
PubKey(157-159)
🪛 Gitleaks (8.26.0)
x/genutil/utils.go
90-90: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
x/genutil/client/cli/init.go (1)
195-196: Default flag value relies on package-private constant – verify it equals"ed25519"
ed25519.KeyTypeis used as the default for--consensus-key-algo.
If this constant is ever renamed (e.g. to matchPrivKeyName) it will silently pass an
unsupported value toInitializeNodeValidatorFilesFromMnemonic.
Consider validating the flag against a whitelist ("ed25519","bls12_381") before use,
or hard-coding the string to avoid drift.crypto/keys/multisig/codec.go (1)
28-30: BLS key registered correctly – good jobThe Amino codec now recognises BLS12-381 public keys, bringing multisig support in line
with the new key type.crypto/codec/proto.go (1)
19-20: LGTM – BLS12-381 key support registered correctlyThe public-key interface now recognises the new BLS type; no issues found.
crypto/keys/bls12_381/key_cgo.go (1)
55-66: 🛠️ Refactor suggestionError from
NewPrivateKeyFromBytesis swallowed
PubKey()discards the error and returnsnil– callers will then dereferenceniland panic elsewhere. Propagate the error instead.-secretKey, err := bls12381.NewPrivateKeyFromBytes(privKey.Key) -if err != nil { - return nil -} +secretKey, err := bls12381.NewPrivateKeyFromBytes(privKey.Key) +if err != nil { + // keep the interface contract by returning an empty pubkey + return &PubKey{} +}Likely an incorrect or invalid review comment.
simapp/simd/cmd/testnet.go
Outdated
| nodeIDs[i], valPubKeys[i], err = genutil.InitializeNodeValidatorFiles(nodeConfig, args.algo) | ||
| if err != nil { | ||
| _ = os.RemoveAll(args.outputDir) |
There was a problem hiding this comment.
Passing arbitrary --key-type values yields silent fallback to Ed25519
InitializeNodeValidatorFiles only distinguishes "ed25519" and "bls12_381".
If the user keeps the default ("secp256k1") the switch will hit default, produce an
Ed25519 key, yet everything downstream (CLI output, genesis PubKeyTypes) will still
say "secp256k1", causing signature-verification failures when the chain starts.
Add upfront validation or map legacy names:
valid := map[string]struct{}{"ed25519": {}, "bls12_381": {}}
if _, ok := valid[args.algo]; !ok {
return fmt.Errorf("unsupported consensus key algorithm %q; choose ed25519 or bls12_381", args.algo)
}Alternatively, default --key-type to "ed25519" for consensus keys.
🤖 Prompt for AI Agents
In simapp/simd/cmd/testnet.go around lines 297 to 299, the code silently falls
back to Ed25519 keys when an unsupported or legacy key type like "secp256k1" is
passed to InitializeNodeValidatorFiles, causing mismatches and verification
failures. To fix this, add upfront validation for args.algo before calling
InitializeNodeValidatorFiles by checking if it is either "ed25519" or
"bls12_381". If not, return an error indicating the unsupported key algorithm
and instruct the user to choose a valid option. Alternatively, set the default
key type to "ed25519" to avoid silent fallback.
| case *cmtprotocrypto.PublicKey_Bls12381: | ||
| return &bls12_381.PubKey{ | ||
| Key: protoPk.Bls12381, | ||
| }, nil | ||
| default: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Edge-case: nil slice handling
protoPk.Bls12381 comes straight from the protobuf message. While Tendermint currently always serialises a 48-byte slice, defensive code usually checks for len(protoPk.Bls12381)==0 and returns an error instead of an empty key object – mirroring the checks done for ed25519 and secp256k1 elsewhere in the codebase.
🤖 Prompt for AI Agents
In crypto/codec/cmt.go around lines 28 to 32, add a defensive check for
protoPk.Bls12381 to verify its length is not zero before returning the
bls12_381.PubKey. If the slice length is zero, return an error indicating an
empty key instead of returning a key object with an empty slice. This aligns
with existing checks for ed25519 and secp256k1 keys in the codebase.
x/genutil/client/cli/collect.go
Outdated
| nodeID, valPubKey, err := genutil.InitializeNodeValidatorFiles(config, consensusKey) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to initialize node validator files") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default key-type handling
If the flag is omitted, consensusKey will be an empty string. InitializeNodeValidatorFiles should treat "" as “use the default algorithm”, but that contract is implicit. Either:
- enforce a default value via the flag (preferred – see previous comment), or
- explicitly set
consensusKey = "ed25519"when the flag is empty.
🤖 Prompt for AI Agents
In x/genutil/client/cli/collect.go around lines 40 to 42, the consensusKey
variable may be an empty string if the flag is omitted, but
InitializeNodeValidatorFiles expects an explicit key type or default. To fix
this, either enforce a default value for the consensusKey flag earlier in the
code or explicitly set consensusKey to "ed25519" before calling
InitializeNodeValidatorFiles when consensusKey is empty, ensuring the default
algorithm is used explicitly.
x/genutil/utils.go
Outdated
| switch keyType { | ||
| case "ed25519": | ||
| keyGenF = nil | ||
| // filePV = loadOrGenFilePV(tmed25519.GenPrivKey(), pvKeyFile, pvStateFile) | ||
| case "bls12_381": | ||
| keyGenF = cmtbls12381.GenPrivKey | ||
| default: | ||
| keyGenF = nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unknown key types silently fall back to ed25519 – prefer explicit failure
If the caller misspells "bls12_381" (e.g. "bls12381"), default: sets keyGenF = nil, CometBFT then generates an ed25519 key.
Silent fallback risks producing an unexpected validator identity.
- default:
- keyGenF = nil // CometBFT treats nil as ed25519
+ default:
+ return "", nil, fmt.Errorf("unsupported consensus key type %q", keyType)🧰 Tools
🪛 Gitleaks (8.26.0)
90-90: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In x/genutil/utils.go around lines 85 to 94, the switch statement silently falls
back to ed25519 by setting keyGenF to nil on unknown key types, which can cause
unexpected validator identities. Modify the default case to explicitly return an
error or panic indicating an unknown key type instead of setting keyGenF to nil,
ensuring the caller is immediately aware of the invalid key type input.
x/genutil/utils.go
Outdated
| } | ||
|
|
||
| // InitializeNodeValidatorFilesFromMnemonic creates private validator and p2p configuration files using the given mnemonic. | ||
| // If no valid mnemonic is given, a random one will be used instead. Key type is ed25519. | ||
| func InitializeNodeValidatorFilesFromMnemonic(config *cfg.Config, mnemonic string) (nodeID string, valPubKey cryptotypes.PubKey, err error) { | ||
| func InitializeNodeValidatorFilesFromMnemonic(config *cfg.Config, mnemonic string, keyType string) (nodeID string, valPubKey cryptotypes.PubKey, err error) { |
There was a problem hiding this comment.
We should keep the InitializeNodeValidatorFilesFromMnemonic and InitializeNodeValidatorFiles functions the same.
And implement new functions InitializeNodeValidatorFilesWithKeyType() that take the extra keytype argument. I don't want us breaking public API surface with this change
aljo242
changed the title
|
@simonzg could we get a CHANGELOG entry for this too? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…alidatorFiles to previous version; add InitializeNodeValidatorFilesWithKeyType and InitializeNodeValidatorFilesFromMnemonicWithKeyType for extra bls12381 key type support
|
where should I put the CHANGELOG entry ? Most of my modifications are in x/genutil, maybe I should create a CHANGELOG.md there? |
Its good to just have it in the root ./CHANGELOG file |
| @@ -0,0 +1,504 @@ | |||
| // Code generated by protoc-gen-gogo. DO NOT EDIT. | |||
There was a problem hiding this comment.
where is this proto coming from?
Does not look like the actual proto is being added in this PR
x/genutil/CHANGELOG.md
Outdated
| @@ -0,0 +1,38 @@ | |||
| <!-- | |||
There was a problem hiding this comment.
lets move to the main changelog
| @@ -98,3 +100,80 @@ func InitializeNodeValidatorFilesFromMnemonic(config *cfg.Config, mnemonic strin | |||
|
|
|||
| return nodeID, valPubKey, nil | |||
| } | |||
|
|
|||
| func InitializeNodeValidatorFilesWithKeyType(config *cfg.Config, keyType string) (nodeID string, valPubKey cryptotypes.PubKey, err error) { | |||
There was a problem hiding this comment.
So now InitializeNodeValidatorFile should call InitializeNodeValidatorFilesWithKeyType with keytype ed25519
|
One more nit:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This is good stuff! Nice work. |
|
need to run |
|
lint-fix is done. The failed test case was |
Port the support for BLS 12381 key in cli
init,gentx,collect-gentxSummary by CodeRabbit