feat(crypto): add native secp256k1 support - #52
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds configurable JNA-backed secp256k1 signing and recovery. It routes EC-key verification through the selected backend, updates PBFT signature handling, and adds configuration, compatibility, validation, and benchmark tests. ChangesNative secp256k1 integration
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This PR adds opt-in native secp256k1 support with fallback behavior while preserving existing defaults; no actionable merge-blocking risk remains in the supplied evidence. Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PBFTHandler
participant SignUtils
participant NativeSecp256k1
participant ECKey
PBFTHandler->>SignUtils: Recover signer address
alt Native EC-key verification enabled
SignUtils->>NativeSecp256k1: Recover public key and address
NativeSecp256k1-->>SignUtils: Return address
else Native verification disabled
SignUtils->>ECKey: Recover public key and address
ECKey-->>SignUtils: Return address
end
SignUtils-->>PBFTHandler: Return signer address
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🧹 Nitpick comments (1)
consensus/src/main/java/org/tron/consensus/pbft/message/PbftBaseMessage.java (1)
99-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the hardcoded engine flag.
The literal
trueselects the EC-key crypto engine. It preserves the previousECKey.signatureToAddressbehavior, so PBFT recovery stays on EC-key even when the node runs the SM2 engine. That intent is not visible at the call site.Add a short comment, or extract a named constant such as
PBFT_USES_ECKEY_ENGINE. The same literal appears inframework/src/main/java/org/tron/core/net/messagehandler/PbftDataSyncHandler.javaat lines 176 to 177.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@consensus/src/main/java/org/tron/consensus/pbft/message/PbftBaseMessage.java` around lines 99 - 100, The hardcoded crypto-engine flag in PbftBaseMessage signature recovery is undocumented. Add a short comment or reuse a clearly named constant such as PBFT_USES_ECKEY_ENGINE at the SignUtils.signatureToAddress call, and apply the same documentation or constant usage to the corresponding call in PbftDataSyncHandler so the intentional EC-key behavior remains explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crypto/build.gradle`:
- Line 16: Update the secp256k1 dependency declaration in the crypto Gradle
configuration: either replace
com.github.federico2014.besu-native:secp256k1:1.3.11 with the official upstream
coordinate, or document the fork’s necessity and build provenance, including who
reviewed it, while preserving the verification metadata alignment.
---
Nitpick comments:
In
`@consensus/src/main/java/org/tron/consensus/pbft/message/PbftBaseMessage.java`:
- Around line 99-100: The hardcoded crypto-engine flag in PbftBaseMessage
signature recovery is undocumented. Add a short comment or reuse a clearly named
constant such as PBFT_USES_ECKEY_ENGINE at the SignUtils.signatureToAddress
call, and apply the same documentation or constant usage to the corresponding
call in PbftDataSyncHandler so the intentional EC-key behavior remains explicit.
🪄 Autofix
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 Plus
Run ID: 26652c53-d3cf-4f18-b34a-210fb360adea
📒 Files selected for processing (16)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/config/args/MiscConfig.javacommon/src/main/resources/reference.confcommon/src/test/java/org/tron/core/config/args/MiscConfigTest.javaconsensus/src/main/java/org/tron/consensus/pbft/message/PbftBaseMessage.javacrypto/build.gradlecrypto/src/main/java/org/tron/common/crypto/NativeSecp256k1.javacrypto/src/main/java/org/tron/common/crypto/SignUtils.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/net/messagehandler/PbftDataSyncHandler.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/ParameterTest.javaframework/src/test/java/org/tron/common/crypto/NativeSecp256k1BenchmarkTest.javaframework/src/test/java/org/tron/common/crypto/NativeSecp256k1Test.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javagradle/verification-metadata.xml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
780fcc6 to
f9bd0f3
Compare
b2bfe99 to
174b6a0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
9e4363e to
efb3768
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
What does this PR do?
Add an optional JNA-backed
NativeSecp256k1implementation for secp256k1 key construction, deterministic signing, public-key recovery, and address recovery.Native signing accepts unsigned private-key encodings from 1 to 32 bytes, as well as valid 33-byte Java BigInteger encodings with a leading sign byte. Private keys are validated against the secp256k1 range [1, n - 1] and normalized to the native API's required 32-byte big-endian representation. Temporary private-key buffers are cleared after use without modifying caller-owned input arrays.
Add the
crypto.useNativeSecp256k1configuration option and route ECKey public-key/address recovery used during signature verification through the native implementation when explicitly enabled. PBFT signature recovery now uses the sharedSignUtilsrouting path.Preserve ECKey recovery compatibility when native verification is enabled:
v = 27..34.1 <= r, s < n; route other legacy scalar combinations directly to ECKey.If native secp256k1 is requested but unavailable, startup now falls back to ECKey instead of terminating. A WARN records the fallback, and startup INFO reports
engine,nativeRequested,nativeActive, and the effective implementation. The runtimeuseNativeSecp256k1value reflects the implementation that is actually active.Add native/ECKey cross-compatibility tests, scalar and recovery-header boundary tests, configuration and logging tests, dependency verification metadata, and an opt-in signing and recovery benchmark.
Why are these changes required?
The existing pure-Java ECKey implementation has significantly higher signing and signature-recovery overhead. Native libsecp256k1 provides a faster implementation while preserving the existing signature format, recovery-ID handling, canonical low-S behavior, recovered TRON addresses, and historical recovery outcomes.
Native acceleration is optional. A missing or unsupported native library should reduce performance rather than prevent a node from starting. Explicit fallback logging and effective-state reporting make this behavior observable to operators.
The feature is disabled by default, so existing nodes retain the original Java implementation unless explicitly configured otherwise.
This PR has been tested by:
NativeSecp256k1TestArgsTest:framework:checkstyleMain:framework:checkstyleTestThe native and Java implementations were cross-tested with deterministic signatures, bidirectional signature recovery, constructor compatibility, high-S signatures, malformed input, scalar boundaries
0,1,n - 1,n, andn + 1, recovery headers27..34, point-at-infinity compatibility, unavailable-native fallback, and configuration routing.The native signing tests also cover short private-key encodings, 33-byte Java BigInteger sign-padded encodings, invalid private-key ranges, and preservation of caller-owned private-key arrays.
A local 10,000-iteration benchmark measured approximately 36.11x faster signing and 17.45x faster signature-address recovery on an x86_64 Java 8 environment (ECKey: 1,404,666 vs NativeSecp256k1: 38,895 ns/op for signing; 894,673 vs 51,259 ns/op for recovery).
CI validation covers x86_64/JDK 8 and aarch64/JDK 17 build environments.
Follow up
None.
Extra details
The new configuration defaults to
falseand is effective only whencrypto.engine = "eckey". SM2 behavior is unchanged. If the native library is unavailable, configured signature recovery and verification remain on ECKey.This change adds JNA 5.12.1 and
io.github.federico2014:secp256k1:1.3.12, including Gradle dependency verification metadata.Summary by CodeRabbit
New Features
Tests