Open
Conversation
📝 WalkthroughWalkthroughThe changes enhance cryptographic key validation and handling across the codebase. ECKey.java and SM2.java undergo significant refactoring to enforce strict key material validation, consolidate ECC parameters into final fields, and introduce defensive copying patterns. Signature validation checks are added to TransactionCapsule and Wallet. SM2Signer switches from random to deterministic HMAC-based K calculation. Test expectations are updated to reflect new exception-throwing behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 10 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="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java:116">
P2: This hardcodes `ECKey.isValidPrivateKey` regardless of the configured crypto engine. When SM2 is in use, this validates against the wrong curve order (SECP256K1N instead of SM2_N). Consider dispatching to the correct validator based on `Args.PARAMETER.isECKeyCryptoEngine()`, or using a method on `SignInterface` if available.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| String prikey = ByteArray.toHexString(sign.getPrivateKey()); | ||
| privateKeys.add(prikey); | ||
| byte[] privateKeyBytes = sign.getPrivateKey(); | ||
| if (!ECKey.isValidPrivateKey(privateKeyBytes)) { |
There was a problem hiding this comment.
P2: This hardcodes ECKey.isValidPrivateKey regardless of the configured crypto engine. When SM2 is in use, this validates against the wrong curve order (SECP256K1N instead of SM2_N). Consider dispatching to the correct validator based on Args.PARAMETER.isECKeyCryptoEngine(), or using a method on SignInterface if available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java, line 116:
<comment>This hardcodes `ECKey.isValidPrivateKey` regardless of the configured crypto engine. When SM2 is in use, this validates against the wrong curve order (SECP256K1N instead of SM2_N). Consider dispatching to the correct validator based on `Args.PARAMETER.isECKeyCryptoEngine()`, or using a method on `SignInterface` if available.</comment>
<file context>
@@ -111,8 +112,12 @@ private void tryInitFromKeystore() {
- String prikey = ByteArray.toHexString(sign.getPrivateKey());
- privateKeys.add(prikey);
+ byte[] privateKeyBytes = sign.getPrivateKey();
+ if (!ECKey.isValidPrivateKey(privateKeyBytes)) {
+ throw new TronError("Private key from keystore is invalid",
+ TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
</file context>
Suggested change
| if (!ECKey.isValidPrivateKey(privateKeyBytes)) { | |
| boolean validKey = Args.PARAMETER.isECKeyCryptoEngine() | |
| ? ECKey.isValidPrivateKey(privateKeyBytes) | |
| : org.tron.common.crypto.sm2.SM2.isValidPrivateKey(privateKeyBytes); | |
| if (!validKey) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crypto/src/main/java/org/tron/common/crypto/ECKey.java (2)
171-187: Redundant validation between constructor and privateKeyFromBigInteger.The private key is validated at line 173 and again at line 246 inside
privateKeyFromBigInteger(). Consider adding a private overload that skips validation for internal trusted calls.♻️ Proposed refactor to avoid redundant validation
public ECKey(byte[] key, boolean isPrivateKey) { if (isPrivateKey) { if (!isValidPrivateKey(key)) { throw new IllegalArgumentException("Invalid private key."); } BigInteger pk = new BigInteger(1, key); - this.privKey = privateKeyFromBigInteger(pk); + this.privKey = privateKeyFromBigIntegerUnchecked(pk); this.pub = CURVE.getG().multiply(pk); } else {And add a private unchecked variant:
private static PrivateKey privateKeyFromBigIntegerUnchecked(BigInteger priv) { try { return ECKeyFactory .getInstance(TronCastleProvider.getInstance()) .generatePrivate(new ECPrivateKeySpec(priv, CURVE_SPEC)); } catch (InvalidKeySpecException ex) { throw new AssertionError("Assumed correct key spec statically"); } }🤖 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/ECKey.java` around lines 171 - 187, The constructor ECKey(byte[] key, boolean isPrivateKey) redundantly validates the private key and then calls privateKeyFromBigInteger() which re-validates; add a private unchecked helper (e.g., privateKeyFromBigIntegerUnchecked(BigInteger priv)) that creates the PrivateKey without re-checking (use ECKeyFactory.getInstance(TronCastleProvider.getInstance()).generatePrivate(new ECPrivateKeySpec(priv, CURVE_SPEC)) and wrap InvalidKeySpecException into an AssertionError), then have the ECKey constructor call this unchecked helper when the key was already validated to avoid duplicate validation.
217-217: Minor formatting: missing space after:in ternary operator.🧹 Fix formatting
- priv == null ? null :privateKeyFromBigInteger(priv), + priv == null ? null : privateKeyFromBigInteger(priv),🤖 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/ECKey.java` at line 217, The ternary expression in ECKey (using variable priv and calling privateKeyFromBigInteger(priv)) is missing a space after the colon; update the expression priv == null ? null :privateKeyFromBigInteger(priv) to include a space after the colon so it reads priv == null ? null : privateKeyFromBigInteger(priv) to match surrounding formatting and style.
🤖 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/Rsv.java`:
- Around line 17-20: The parser currently only checks for sign.length < 65 and
therefore accepts and silently truncates overlong signatures; update the check
in org.tron.common.crypto.Rsv (the method/constructor that takes the byte[] sign
and later slices r/s/v on lines 21-23) to reject any length that is not exactly
65 (i.e., throw IllegalArgumentException when sign == null || sign.length != 65)
so trailing bytes are not accepted or silently ignored.
In `@framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java`:
- Around line 115-120: The current keystore validation in WitnessInitializer
uses ECKey.isValidPrivateKey(...) unconditionally; change it to branch on the
active crypto engine (use Args.PARAMETER.isECKeyCryptoEngine() or inspect the
loaded SignInterface implementation referenced by the local sign variable) and
call the appropriate validation for the selected engine instead of ECKey for SM2
keys (i.e., only call ECKey.isValidPrivateKey when the engine is EC; otherwise
validate using the SM2-specific key validator provided by the SM2 SignInterface
implementation). Ensure the thrown TronError remains the same on invalid keys
and continue adding the validated key to privateKeys as before.
---
Nitpick comments:
In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java`:
- Around line 171-187: The constructor ECKey(byte[] key, boolean isPrivateKey)
redundantly validates the private key and then calls privateKeyFromBigInteger()
which re-validates; add a private unchecked helper (e.g.,
privateKeyFromBigIntegerUnchecked(BigInteger priv)) that creates the PrivateKey
without re-checking (use
ECKeyFactory.getInstance(TronCastleProvider.getInstance()).generatePrivate(new
ECPrivateKeySpec(priv, CURVE_SPEC)) and wrap InvalidKeySpecException into an
AssertionError), then have the ECKey constructor call this unchecked helper when
the key was already validated to avoid duplicate validation.
- Line 217: The ternary expression in ECKey (using variable priv and calling
privateKeyFromBigInteger(priv)) is missing a space after the colon; update the
expression priv == null ? null :privateKeyFromBigInteger(priv) to include a
space after the colon so it reads priv == null ? null :
privateKeyFromBigInteger(priv) to match surrounding formatting and style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8f0416c-7a19-482e-9023-ea756f2fd05b
📒 Files selected for processing (10)
chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.javacrypto/src/main/java/org/tron/common/crypto/ECKey.javacrypto/src/main/java/org/tron/common/crypto/Rsv.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/test/java/org/tron/common/crypto/ECKeyTest.javaframework/src/test/java/org/tron/common/crypto/SM2KeyTest.javaframework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
💤 Files with no reviewable changes (1)
- framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
Comment on lines
+17
to
+20
| if (sign == null || sign.length < 65) { | ||
| throw new IllegalArgumentException( | ||
| "Invalid signature length: " + (sign == null ? "null" : sign.length)); | ||
| } |
There was a problem hiding this comment.
Reject overlong signatures too.
This still accepts 66+ byte inputs, and Lines 21-23 will silently truncate them into a valid-looking r/s/v tuple. Higher layers can keep their SignatureFormatException mapping at the boundary, but this parser itself should not accept trailing bytes at all.
Suggested fix
- if (sign == null || sign.length < 65) {
+ if (sign == null || sign.length != 65) {
throw new IllegalArgumentException(
"Invalid signature length: " + (sign == null ? "null" : sign.length));
}🤖 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/Rsv.java` around lines 17 - 20,
The parser currently only checks for sign.length < 65 and therefore accepts and
silently truncates overlong signatures; update the check in
org.tron.common.crypto.Rsv (the method/constructor that takes the byte[] sign
and later slices r/s/v on lines 21-23) to reject any length that is not exactly
65 (i.e., throw IllegalArgumentException when sign == null || sign.length != 65)
so trailing bytes are not accepted or silently ignored.
Comment on lines
+115
to
+120
| byte[] privateKeyBytes = sign.getPrivateKey(); | ||
| if (!ECKey.isValidPrivateKey(privateKeyBytes)) { | ||
| throw new TronError("Private key from keystore is invalid", | ||
| TronError.ErrCode.WITNESS_KEYSTORE_LOAD); | ||
| } | ||
| privateKeys.add(ByteArray.toHexString(privateKeyBytes)); |
There was a problem hiding this comment.
Use the active crypto engine when validating keystore keys.
This always validates against ECKey, so an SM2 witness node will reject a valid SM2 keystore key against the wrong curve order. Branch on Args.PARAMETER.isECKeyCryptoEngine() (or the loaded SignInterface type) before validating.
Suggested fix
+import org.tron.common.crypto.sm2.SM2;
...
- if (!ECKey.isValidPrivateKey(privateKeyBytes)) {
+ boolean validPrivateKey = Args.PARAMETER.isECKeyCryptoEngine()
+ ? ECKey.isValidPrivateKey(privateKeyBytes)
+ : SM2.isValidPrivateKey(privateKeyBytes);
+ if (!validPrivateKey) {
throw new TronError("Private key from keystore is invalid",
TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| byte[] privateKeyBytes = sign.getPrivateKey(); | |
| if (!ECKey.isValidPrivateKey(privateKeyBytes)) { | |
| throw new TronError("Private key from keystore is invalid", | |
| TronError.ErrCode.WITNESS_KEYSTORE_LOAD); | |
| } | |
| privateKeys.add(ByteArray.toHexString(privateKeyBytes)); | |
| byte[] privateKeyBytes = sign.getPrivateKey(); | |
| boolean validPrivateKey = Args.PARAMETER.isECKeyCryptoEngine() | |
| ? ECKey.isValidPrivateKey(privateKeyBytes) | |
| : SM2.isValidPrivateKey(privateKeyBytes); | |
| if (!validPrivateKey) { | |
| throw new TronError("Private key from keystore is invalid", | |
| TronError.ErrCode.WITNESS_KEYSTORE_LOAD); | |
| } | |
| privateKeys.add(ByteArray.toHexString(privateKeyBytes)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java`
around lines 115 - 120, The current keystore validation in WitnessInitializer
uses ECKey.isValidPrivateKey(...) unconditionally; change it to branch on the
active crypto engine (use Args.PARAMETER.isECKeyCryptoEngine() or inspect the
loaded SignInterface implementation referenced by the local sign variable) and
call the appropriate validation for the selected engine instead of ECKey for SM2
keys (i.e., only call ECKey.isValidPrivateKey when the engine is EC; otherwise
validate using the SM2-specific key validator provided by the SM2 SignInterface
implementation). Ensure the thrown TronError remains the same on invalid keys
and continue adding the validated key to privateKeys as before.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java (1)
85-90:⚠️ Potential issue | 🟠 MajorUse engine-specific private-key validation in keystore flow.
Line 86 always validates with
ECKey, so a valid SM2 keystore key can be rejected when SM2 is the active engine, causing witness init failure.Suggested fix
import org.tron.common.crypto.ECKey; import org.tron.common.crypto.SignInterface; +import org.tron.common.crypto.sm2.SM2; @@ SignInterface sign = credentials.getSignInterface(); byte[] privateKeyBytes = sign.getPrivateKey(); - if (!ECKey.isValidPrivateKey(privateKeyBytes)) { + boolean validPrivateKey = Args.getInstance().isECKeyCryptoEngine() + ? ECKey.isValidPrivateKey(privateKeyBytes) + : SM2.isValidPrivateKey(privateKeyBytes); + if (!validPrivateKey) { throw new TronError("Private key from keystore is invalid", TronError.ErrCode.WITNESS_KEYSTORE_LOAD); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java` around lines 85 - 90, The keystore flow in WitnessInitializer currently always calls ECKey.isValidPrivateKey(privateKeyBytes), which will incorrectly reject SM2 keys when SM2 is the active crypto engine; replace that hard-coded ECKey check with an engine-aware validator: query the active crypto engine (use your project's crypto/crypto-config helper or engine accessor) and call the appropriate validator (e.g., ECKey.isValidPrivateKey(...) for ECDSA or the SM2 validator for SM2), or invoke a central helper like CryptoValidator.isValidPrivateKeyForEngine(byte[] key) if available; keep the existing error throw (TronError with ErrCode.WITNESS_KEYSTORE_LOAD) and then add the hex string to privateKeys as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java`:
- Around line 85-90: The keystore flow in WitnessInitializer currently always
calls ECKey.isValidPrivateKey(privateKeyBytes), which will incorrectly reject
SM2 keys when SM2 is the active crypto engine; replace that hard-coded ECKey
check with an engine-aware validator: query the active crypto engine (use your
project's crypto/crypto-config helper or engine accessor) and call the
appropriate validator (e.g., ECKey.isValidPrivateKey(...) for ECDSA or the SM2
validator for SM2), or invoke a central helper like
CryptoValidator.isValidPrivateKeyForEngine(byte[] key) if available; keep the
existing error throw (TronError with ErrCode.WITNESS_KEYSTORE_LOAD) and then add
the hex string to privateKeys as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42672cee-6c26-4bb9-aa64-0c5ad4cad90b
📒 Files selected for processing (1)
framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java
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.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Strengthens ECDSA and SM2 signing/verification with strict key/signature validation and deterministic
SM2signatures. Improves signature handling in transactions and keystore loading with clearer errors and safer returns.New Features
SM2Signernow uses deterministic nonces viaHMacDSAKCalculatorwithSM3(RFC 6979-style).ECKey.isValidPrivateKey,ECKey.isValidPublicKey, and SM2 equivalents; constructors and factories enforce validity.Bug Fixes
WalletandTransactionCapsulewith clear messages.Rsv.fromSignatureinput length and throw on invalid payloads.recId,r,s, and 32-byte hash; use curve order consistently; handles + r == 0inSM2.getAddress()andgetNodeId(); caches arevolatileto avoid races.WitnessInitializer; fail fast withTronError.ECKey.fromPrivate(byte[])andSM2.fromPrivate(byte[])now throw on invalid/empty input (tests updated).Written for commit 3b8923f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests