feat: add support for ed25519 tx signature verification
#23283
Conversation
|
Warning Rate limit exceeded@zivkovicmilos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces support for ed25519 transaction signatures in the Cosmos SDK. The changes span multiple files, including Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
zivkovicmilos
requested review from
a team,
JulianToledano and
raynaudoe
as code owners
x/auth/ante/sigverify.go
Outdated
| @@ -112,18 +110,16 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool { | |||
| } | |||
|
|
|||
| func (svd SigVerificationDecorator) VerifyIsOnCurve(pubKey cryptotypes.PubKey) error { | |||
| if svd.extraVerifyIsOnCurve != nil { | |||
There was a problem hiding this comment.
Please revert this change. It is always nil if you use NewSigVerificationDecorator but if you use the NewSigVerificationDecoratorWithVerifyOnCurve you can specify one.
This is a feature that just got added (#23128) :D
There was a problem hiding this comment.
Ah, didn't realize it's relatively new 🤦♂️
I just saw that it's always nil, so I dropped it.
Reverted:
![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
🧹 Nitpick comments (3)
CHANGELOG.md (3)
Line range hint
1-1: Add a title to the CHANGELOG fileThe file should start with a descriptive title like "# Changelog" to clearly identify its purpose.
+ # Changelog
48-48: Fix inconsistent version header formattingThe version header for v0.47.15 is missing the release date in parentheses, unlike other version headers.
- ## [v0.47.15](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.15) + ## [v0.47.15](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.15) - 2024-12-16
Line range hint
785-785: Improve clarity of breaking change descriptionThe description "Bump Go version minimum requirement to 1.19" should include the previous version for better context.
- * (deps) [#13397](https://github.com/cosmos/cosmos-sdk/pull/13397) Bump Go version minimum requirement to `1.19`. + * (deps) [#13397](https://github.com/cosmos/cosmos-sdk/pull/13397) Update minimum required Go version from 1.18 to 1.19.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)crypto/keys/ed25519/ed25519.go(4 hunks)crypto/keys/ed25519/ed25519_test.go(2 hunks)go.mod(1 hunks)x/auth/ante/sigverify.go(3 hunks)x/auth/ante/sigverify_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
x/auth/ante/sigverify_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/keys/ed25519/ed25519_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/keys/ed25519/ed25519.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/sigverify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (9)
crypto/keys/ed25519/ed25519.go (2)
235-238: Well-documented constant for curve validation!The
identityPointvariable is properly documented and correctly represents the neutral element (0,1) in Edwards curve form.
240-264: Robust implementation of curve validation!The
IsOnCurvemethod implements comprehensive validation:
- Validates key size
- Ensures point is on the curve
- Rejects identity point (neutral element)
This implementation aligns with cryptographic best practices for ed25519 key validation.
crypto/keys/ed25519/ed25519_test.go (1)
260-295: Excellent test coverage for curve validation!The test suite comprehensively covers all critical scenarios:
- Invalid public key size
- Identity point rejection
- Valid public key acceptance
Good use of
t.Parallel()for test efficiency.x/auth/ante/sigverify.go (2)
119-122: Clean integration of ed25519 curve validation!The implementation properly integrates the new
IsOnCurvecheck for ed25519 keys, maintaining consistency with other key type validations.
533-533: Proper gas consumption for ed25519 signatures!The implementation now correctly consumes gas for ed25519 signature verification without raising an error, enabling support for ed25519 transaction signatures.
x/auth/ante/sigverify_test.go (1)
77-77: Test expectation correctly updated!The test case for ed25519 signature verification now expects success, aligning with the implementation changes that support ed25519 transaction signatures.
go.mod (2)
19-19: Verify the selected version of edwards25519.The addition of
filippo.io/edwards25519as a direct dependency is appropriate for supporting ed25519 signature verification. However, let's verify if v1.1.0 is the most suitable version.Run this script to check available versions and their release dates:
✅ Verification successful
The selected version v1.1.0 of edwards25519 is appropriate.
The codebase uses only stable, core functionality (point operations, curve validation) that is well-supported in v1.1.0. The integration with CometBFT's crypto packages is clean and shows no compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check available versions of edwards25519 and their release dates gh api \ -H "Accept: application/vnd.github+json" \ repos/FiloSottile/edwards25519/releases \ --jq '.[] | {tag_name: .tag_name, published_at: .published_at}'Length of output: 153
Script:
#!/bin/bash # Search for edwards25519 imports and usage patterns echo "=== Searching for edwards25519 imports ===" rg -l "filippo\.io/edwards25519" --type go echo -e "\n=== Checking for version-specific code patterns ===" rg "edwards25519" --type go -C 2 echo -e "\n=== Looking for documentation mentioning edwards25519 ===" fd -e md -e txt | xargs rg "edwards25519"Length of output: 2197
19-19: Verify compatibility with existing crypto packages.The addition looks good, but let's ensure there are no conflicts with other cryptographic dependencies in use.
Run this script to check for potential conflicts:
✅ Verification successful
Dependency is properly integrated and compatible
The
filippo.io/edwards25519 v1.1.0package is already well-integrated into the codebase, used specifically for ed25519 curve operations in the crypto/keys/ed25519 package. There are no conflicts with other cryptographic dependencies, and the version is consistent throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other crypto-related dependencies that might conflict rg -l "ed25519|curve25519|edwards25519" | grep -v "go.mod" # Check imports in crypto-related files ast-grep --pattern 'import ($$$)' | grep -i "crypto\|ed25519\|curve25519"Length of output: 65912
Script:
#!/bin/bash # Check go.sum for edwards25519 versions echo "=== Checking go.sum for edwards25519 versions ===" grep "filippo.io/edwards25519" go.sum # Look for direct imports of edwards25519 echo -e "\n=== Checking direct imports of edwards25519 ===" rg --type go "filippo.io/edwards25519" -l # Look for actual usage of edwards25519 types/functions echo -e "\n=== Checking usage of edwards25519 package ===" rg --type go "edwards25519\." -B 2 -A 2Length of output: 1753
CHANGELOG.md (1)
Line range hint
1-2500: Overall documentation quality is goodThe CHANGELOG follows proper semantic versioning, includes relevant categorization of changes, and provides good traceability through PR links. The descriptions are generally clear and informative.
Description
Closes: #1861 (for real)
This PR introduces support for
ed25519transaction signing.The original issue was brought up in #1861, but still the support remained missing in the SDK.
It also promotes a dependency from indirect to direct (
filippo.io/edwards25519), since it has handy methods for verifyinged25519public keys.There is logic for subtracting gas from the gas meter when an
ed25519signature is encountered, but a simple error is always returned:cosmos-sdk/x/auth/ante/sigverify.go
Lines 537 to 540 in 80b1422
The change is backwards compatible, as it doesn't break existing SDK logic.
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
Release Notes
New Features
Improvements
Dependency Updates
filippo.io/edwards25519packageThese changes enhance the cryptographic signature handling and provide more robust key validation in the transaction signing process.