refactor(crypto): remove deprecated methods and harden SM2/SignUtils#9
Open
Federico2014 wants to merge 1 commit intodevelopfrom
Open
refactor(crypto): remove deprecated methods and harden SM2/SignUtils#9Federico2014 wants to merge 1 commit intodevelopfrom
Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
…den SM2/SignUtils
📝 WalkthroughWalkthroughThe PR removes point compression/decompression utility methods from ECKey and SM2 classes, adds runtime type validation in SignUtils to enforce correct signature types for their respective crypto engines, and makes SM2 elliptic-curve domain constants immutable. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SignUtils
participant ECKey
participant SM2
Client->>SignUtils: signatureToAddress(messageHash, signature, isECKeyCryptoEngine)
alt isECKeyCryptoEngine = true
SignUtils->>SignUtils: validate signature is ECDSASignature
SignUtils->>ECKey: signatureToAddress(messageHash, ecdsaSig)
ECKey-->>SignUtils: address
else isECKeyCryptoEngine = false
SignUtils->>SignUtils: validate signature is SM2Signature
SignUtils->>SM2: signatureToAddress(messageHash, sm2Sig)
SM2-->>SignUtils: address
end
SignUtils-->>Client: address
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crypto/src/main/java/org/tron/common/crypto/SignUtils.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/SignUtils.java:54">
P2: Null `signatureInterface` causes a `NullPointerException` while building the new type-mismatch message, instead of the intended `IllegalArgumentException`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!(signatureInterface instanceof ECDSASignature)) { | ||
| throw new IllegalArgumentException( | ||
| "Expected ECDSASignature for ECKey engine, got: " | ||
| + signatureInterface.getClass().getName()); |
There was a problem hiding this comment.
P2: Null signatureInterface causes a NullPointerException while building the new type-mismatch message, instead of the intended IllegalArgumentException.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/SignUtils.java, line 54:
<comment>Null `signatureInterface` causes a `NullPointerException` while building the new type-mismatch message, instead of the intended `IllegalArgumentException`.</comment>
<file context>
@@ -48,8 +48,18 @@ public static byte[] signatureToAddress(
+ if (!(signatureInterface instanceof ECDSASignature)) {
+ throw new IllegalArgumentException(
+ "Expected ECDSASignature for ECKey engine, got: "
+ + signatureInterface.getClass().getName());
+ }
return ECKey.signatureToAddress(messageHash, (ECDSASignature) signatureInterface);
</file context>
Suggested change
| + signatureInterface.getClass().getName()); | |
| + (signatureInterface == null ? "null" : signatureInterface.getClass().getName())); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypto/src/main/java/org/tron/common/crypto/SignUtils.java`:
- Around line 51-55: Check for null before calling signatureInterface.getClass()
in SignUtils: when validating that signatureInterface is an instance of
ECDSASignature, replace the direct getClass() call in the error message with a
null-safe expression (e.g., use (signatureInterface == null ? "null" :
signatureInterface.getClass().getName())) or explicitly branch to throw an
IllegalArgumentException stating the parameter is null; apply the same
null-guard change to the second occurrence mentioned so neither the instanceof
check nor the error formatting can cause a NullPointerException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e22e54c-4b89-4830-ad7b-e88639c15d97
📒 Files selected for processing (3)
crypto/src/main/java/org/tron/common/crypto/ECKey.javacrypto/src/main/java/org/tron/common/crypto/SignUtils.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2.java
💤 Files with no reviewable changes (1)
- crypto/src/main/java/org/tron/common/crypto/ECKey.java
Comment on lines
+51
to
+55
| if (!(signatureInterface instanceof ECDSASignature)) { | ||
| throw new IllegalArgumentException( | ||
| "Expected ECDSASignature for ECKey engine, got: " | ||
| + signatureInterface.getClass().getName()); | ||
| } |
There was a problem hiding this comment.
Add explicit null guard for signatureInterface before type-check error formatting.
At Line 54 and Line 61, signatureInterface.getClass() can throw NullPointerException when input is null, so the new descriptive IllegalArgumentException is bypassed.
Suggested fix
public static byte[] signatureToAddress(
byte[] messageHash, SignatureInterface signatureInterface, boolean isECKeyCryptoEngine)
throws SignatureException {
+ if (signatureInterface == null) {
+ throw new IllegalArgumentException("signatureInterface must not be null");
+ }
if (isECKeyCryptoEngine) {
if (!(signatureInterface instanceof ECDSASignature)) {
throw new IllegalArgumentException(
"Expected ECDSASignature for ECKey engine, got: "
+ signatureInterface.getClass().getName());Also applies to: 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypto/src/main/java/org/tron/common/crypto/SignUtils.java` around lines 51 -
55, Check for null before calling signatureInterface.getClass() in SignUtils:
when validating that signatureInterface is an instance of ECDSASignature,
replace the direct getClass() call in the error message with a null-safe
expression (e.g., use (signatureInterface == null ? "null" :
signatureInterface.getClass().getName())) or explicitly branch to throw an
IllegalArgumentException stating the parameter is null; apply the same
null-guard change to the second occurrence mentioned so neither the instanceof
check nor the error formatting can cause a NullPointerException.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
compressPoint/decompressPointfrom bothECKeyandSM2(all 4 were
@deprecated, zero callers confirmed by grep)private static finalto SM2 curve domain parameters(
SM2_N,SM2_P,SM2_A,SM2_B,SM2_GX,SM2_GY)instanceoftype guards inSignUtils.signatureToAddressto replace silent
ClassCastExceptionwith descriptiveIllegalArgumentExceptionon engine/signature-type mismatchCompatibility
Removing
compressPoint/decompressPointis a binary-incompatiblechange. External code calling these methods via direct reference will
encounter
NoSuchMethodErrorat runtime after upgrading. Migration:use
ECPoint.getEncoded(boolean)directly.Test plan
./gradlew :framework:test --tests "org.tron.common.crypto.*"— all pass./gradlew :framework:test --tests "org.tron.core.net.messagehandler.PbftMsgHandlerTest"— pass./gradlew :framework:test --tests "org.tron.core.pbft.PbftApiTest"— pass./gradlew :chainbase:test— all passSummary by cubic
Removes deprecated point-compression helpers and adds strict signature-type checks to prevent silent casting errors. Also makes SM2 curve parameters immutable for safer crypto operations.
Refactors
compressPoint/decompressPointfromECKeyandSM2(no callers).instanceofchecks inSignUtils.signatureToAddressand throwIllegalArgumentExceptionon engine/signature mismatch.SM2_N,SM2_P,SM2_A,SM2_B,SM2_GX,SM2_GY) asprivate static final.Migration
NoSuchMethodError.ECPoint.getEncoded(boolean)for point compression/decompression.Written for commit c8adb6c. Summary will update on new commits.
Summary by CodeRabbit