fix(crypto): bls compilation #22717
fix(crypto): bls compilation #22717
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve enhancements to the build process and modifications to cryptographic functions within the SimApp project. Specifically, new build steps for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)crypto/keys/bls12_381/key_cgo.go (1)Pattern 🔇 Additional comments (5).github/workflows/build.yml (1)
The new build steps for BLS12381 and Secp_cgo are correctly configured:
Let's verify the build options are properly recognized: ✅ Verification successfulBuild options for BLS12381 and Secp are properly configured and recognized The build system correctly handles both cryptographic options:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the build system recognizes the new build options
# Check if build options are defined in build configuration
rg -l "bls12381|secp" scripts/build/
Length of output: 61 Script: #!/bin/bash
# Check the content of build.mk to verify how these options are handled
cat scripts/build/build.mk
Length of output: 4430 crypto/keys/bls12_381/key_cgo.go (4)Line range hint The build tag ensures the code only compiles on supported platforms and architectures, which is crucial for cryptographic implementations.
The removal of sha256 import and repositioning of tmhash import aligns with the changes in signing logic. Line range hint The signature verification logic remains robust with proper error handling, but we should verify that the removal of message length handling is consistent with the signing changes. Let's check for consistency in message handling: ✅ Verification successfulBLS signature verification is consistent with signing implementation The verification implementation in
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify consistent message handling across the codebase
# Search for BLS signature verification implementations
ast-grep --pattern 'func $x($y) bool {
$$$
VerifySignature($$$)
$$$
}'
Length of output: 78 Script: #!/bin/bash
# Let's try a different approach to find signature verification implementations
# Search for files containing VerifySignature
rg -l "VerifySignature"
# Then look for actual implementations with context
rg "VerifySignature" -A 5 -B 5
# Also search for message handling in BLS-related files
rg "msg" crypto/keys/bls12_381/
Length of output: 46484 Line range hint The removal of message length check and SHA256 hashing for large messages represents a significant change in the signing behavior. Please verify:
Let's check for any existing length-dependent message handling: ✅ Verification successfulLet me gather more information about the BLS signing implementation and its dependencies. Let me check the underlying BLS library implementation to understand the message handling behavior. Let me check for any test files or documentation that might reveal the intended behavior of BLS signing. Let me check the BLS library implementation and any related test files to understand the message handling behavior. Based on the gathered information, I can now provide a final response about the security implications of the change. Verify that BLS signature implementation aligns with standards and security best practices The removal of message length check and SHA256 hashing for large messages appears to be a deliberate change to align with the underlying BLS library's native message handling. The evidence shows:
This change:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Look for other message length checks in crypto implementations
# Search for message length checks in crypto code
rg -A 3 "len\(msg\)" crypto/
Length of output: 976 Script: #!/bin/bash
# Check BLS signature implementation details and dependencies
# Look for BLS signature related code and message handling
rg -A 5 "Sign.*msg.*byte" crypto/keys/bls12_381/
# Check for MaxMsgLen constant and its usage
rg "MaxMsgLen" crypto/
# Look for any SHA256 hashing in BLS signing context
rg -A 3 "SHA256|Hash" crypto/keys/bls12_381/
# Check for any test files that might reveal the expected behavior
fd ".*test.*" crypto/keys/bls12_381/
Length of output: 2555 Script: #!/bin/bash
# Check the underlying BLS library implementation
# Look for the bls12381 package imports
rg "bls12381" crypto/keys/bls12_381/ -A 3
# Check for any documentation or implementation details in the BLS library
fd "bls12381" . --type f --exec cat {}
# Look for any aggregated signature functionality that might affect message handling
rg -A 3 "Aggregate|BatchVerify" .
Length of output: 69068 Script: #!/bin/bash
# Look for test files and documentation
# Search for test files containing BLS signature tests
fd -e go ".*test.*" . --exec rg -l "Sign.*msg" {}
# Look for any documentation about message handling in BLS signatures
rg -A 3 "BLS|sign.*message" . --type md
# Check for any constants related to message length or hashing
rg "MaxMsgLen|SHA256|Hash" crypto/keys/bls12_381/
Length of output: 31373 Script: #!/bin/bash
# Check BLS library implementation and tests
# Look for the BLS library import path
rg "github.com/cometbft/cometbft/crypto/bls12381" -A 5
# Check for any test files that might show message handling
fd -e go ".*test.*" . --exec rg -l "bls12381" {}
# Look for any message length or hash related constants
rg "SignatureLength|PrivKeySize|PubKeySize" crypto/keys/bls12_381/
Length of output: 3023 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
|
tac0turtle
requested review from
auricom,
julienrbrt,
JulianToledano,
raynaudoe and
a team
as code owners
|
@tac0turtle your pull request is missing a changelog! |
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crypto/keys/bls12_381/key_cgo.go (1)
Line range hint
142-157: Consider adding performance metrics for signature verification.The direct verification of signatures without pre-hashing large messages might impact performance. Consider adding metrics to monitor the verification time for different message sizes.
Would you like me to help implement performance metrics for signature operations?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)crypto/keys/bls12_381/key_cgo.go(1 hunks)scripts/build/build.mk(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/build/build.mk
🧰 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 (3)
.github/workflows/build.yml (1)
58-63: LGTM! Build steps for cryptographic modules look good.
The addition of BLS12381 and Secp_cgo build steps is well-structured and properly conditioned for amd64 architecture. This aligns with the PR's objective of fixing BLS compilation issues.
Let's verify the build options are consistently defined:
✅ Verification successful
Build options for BLS12381 and Secp are properly configured in the build system
The build options in the workflow file are correctly defined and match the build system configuration. In scripts/build/build.mk:
bls12381option adds the corresponding build tag and enables CGOsecpoption adds thelibsecp256k1_sdkbuild tag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build options in makefile
# Check if the build options (bls12381, secp) are properly defined in the build system
rg -l 'COSMOS_BUILD_OPTIONS.*bls12381|COSMOS_BUILD_OPTIONS.*secp' 'Makefile*' 'scripts/build/*'
Length of output: 203
Script:
#!/bin/bash
# Let's first find the Makefile and build scripts in the repository
fd -t f "Makefile|\.mk$"
# Then search for build scripts
fd -t f . "scripts/"
Length of output: 1315
Script:
#!/bin/bash
# Check build.mk for build options configuration
cat scripts/build/build.mk
# Check main Makefile for build options
cat Makefile
# Search for any BLS or secp related build configurations
rg -i "bls|secp.*build" scripts/build/ Makefile
Length of output: 5210
crypto/keys/bls12_381/key_cgo.go (2)
12-12: LGTM! Import change looks good.
The switch from crypto/sha256 to tmhash is appropriate as it aligns with the CometBFT crypto framework.
Line range hint 77-86: Verify the security implications of removing message length checks.
The removal of message length validation and pre-hashing for large messages could have security implications. Please ensure this change:
- Doesn't expose the system to potential DoS attacks with large messages
- Aligns with BLS12-381 signature scheme specifications
- Maintains compatibility with existing signed messages
Let's check for any security considerations in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crypto/keys/bls12_381/key_cgo.go (1)
Line range hint
1-190: Consider adding documentation about security properties.The removal of message hashing represents a significant change in the cryptographic properties of the signing mechanism. This should be clearly documented.
Add documentation explaining:
- The direct signing behavior
- Any message size limitations
- Security considerations for protocol designers
//go:build ((linux && amd64) || (linux && arm64) || (darwin && amd64) || (darwin && arm64) || (windows && amd64)) && bls12381 package bls12_381 + +// Package bls12_381 implements BLS signatures with direct message signing. +// Important security considerations: +// - Messages are signed directly without pre-hashing +// - No internal message length limits are enforced +// - Protocol designers should carefully consider message size implications
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)crypto/keys/bls12_381/key_cgo.go(1 hunks)scripts/build/build.mk(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/build/build.mk
🧰 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 (5)
.github/workflows/build.yml (1)
58-63: LGTM! Build steps for cryptographic modules look correct.
The new build steps for BLS12381 and Secp_cgo are properly configured:
- Correctly restricted to amd64 architecture
- Follows existing pattern of conditional builds
- Build options are properly passed via environment variables
Let's verify the build options are properly recognized:
✅ Verification successful
Build options are properly integrated in the build system
The build system correctly recognizes both bls12381 and secp build options in scripts/build/build.mk:
secpoption addslibsecp256k1_sdkbuild tagbls12381option enables CGO and addsbls12381build tag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the build system recognizes the new build options
# Check if the build options are defined in the build system
rg -A 3 'bls12381|secp' scripts/build/
Length of output: 616
crypto/keys/bls12_381/key_cgo.go (4)
12-12: Verify tmhash import usage after reorganization.
The tmhash import is now only used for Address() calculation. This is correct as it maintains consistent address derivation.
Line range hint 1-1: Verify build tag constraints are sufficient.
The build tag correctly restricts the implementation to supported platforms. However, we should ensure this aligns with all BLS-dependent features.
✅ Verification successful
Let me gather more information about the BLS implementation and its dependencies.
Build tag constraints are correctly implemented
The build tag implementation is well-structured and sufficient:
key_cgo.gocontains the actual BLS implementation with proper platform constraints((linux && amd64) || (linux && arm64) || (darwin && amd64) || (darwin && arm64) || (windows && amd64)) && bls12381key.gohas a complementary constraint!bls12381and contains stub implementations that panic when BLS is not enabled- No other files in the codebase have BLS-specific build constraints, which is correct as they should be platform-agnostic
This setup ensures that:
- BLS functionality is only available on supported platforms when explicitly enabled
- Compilation fails gracefully on unsupported platforms
- Other parts of the codebase remain platform-independent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build tag consistency across BLS implementation
# Check for other BLS-related files and their build tags
fd -e go | xargs rg -l 'bls12381'
# Look for potential platform-specific code
rg -B 1 'go:build.*(linux|darwin|windows).*bls'
Length of output: 421
Script:
#!/bin/bash
# Check the content of key.go and key_cgo.go to understand the implementation structure
cat crypto/keys/bls12_381/key.go
cat crypto/keys/bls12_381/key_cgo.go
# Check for any other build constraints in the codebase related to BLS
rg -p "//go:build.*bls"
# Look for any BLS-related test files
fd -e go | xargs rg -l 'TestBLS|test.*bls'
Length of output: 9473
Line range hint 134-147: Verify signature verification changes align with signing changes.
The VerifySignature implementation has been simplified to match the new signing behavior. While this maintains consistency, we should ensure all consumers are aware of the change.
Line range hint 76-84: Critical: Verify security implications of direct message signing.
The removal of message length checks and direct signing of raw messages instead of their hashes has important security implications:
- Large messages could impact performance
- Changed security properties might affect existing protocols
Let's check for potential impacts:
(cherry picked from commit 9d9c19c)
Co-authored-by: Marko <[email protected]>
Description
This pr fixes bls compilation, not sure how it was missed in the last pr.
This also adds a build for bls and secp to make sure we avoid this in the future
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
amd64architecture, enhancing the build process with specific configurations.Bug Fixes
SignandVerifySignaturemethods.Chores
cleveldbandbadgerdb, streamlining the build process.