build: moving from secp256k1 to k256 #243
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis pull request replaces the use of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyManager as SigningKey Manager
participant K256Signer as k256::ecdsa::SigningKey
participant K256Verifier as k256::ecdsa::VerifyingKey
Client->>KeyManager: Request new secp256k1 key
KeyManager->>K256Signer: Generate key using random method
K256Signer-->>KeyManager: Return generated key
Client->>KeyManager: Submit message for signing
KeyManager->>K256Signer: Call sign_digest(message)
K256Signer-->>KeyManager: Return signature
Client->>K256Verifier: Provide message and signature for verification
K256Verifier->>Client: Execute verify_digest and return result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
CodeRabbit Configuration File (
|
![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 (2)
crates/keys/src/signing_keys.rs (1)
3-5: Consider renaming the alias for clarity.
RenamingDigestSigner as P256DigestSignermight help avoid confusion, as "P256" typically refers to the secp256r1 (p256) curve rather than the secp256k1 (k256) curve.Proposed rename:
-use k256::ecdsa::{ - signature::DigestSigner as P256DigestSigner, Signature as Secp256k1Signature, - SigningKey as Secp256k1SigningKey, +use k256::ecdsa::{ + signature::DigestSigner as K256DigestSigner, Signature as Secp256k1Signature, + SigningKey as Secp256k1SigningKey,crates/keys/src/verifying_keys.rs (1)
93-97: DER-based parsing for secp256k1.
Consistently usingfrom_public_key_der()for secp256k1 ensures standard DER support. Validate that any older secp256k1 keys in non-DER form are migrated if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(2 hunks)crates/keys/Cargo.toml(1 hunks)crates/keys/src/algorithm.rs(0 hunks)crates/keys/src/signatures.rs(3 hunks)crates/keys/src/signing_keys.rs(4 hunks)crates/keys/src/tests.rs(2 hunks)crates/keys/src/verifying_keys.rs(7 hunks)crates/zk/sp1-script/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- crates/keys/src/algorithm.rs
🧰 Additional context used
🧠 Learnings (1)
crates/keys/src/verifying_keys.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#150
File: crates/common/src/keys.rs:27-31
Timestamp: 2024-11-12T11:47:59.930Z
Learning: In the Rust codebase of `prism`, within the `VerifyingKey::as_bytes` method in `crates/common/src/keys.rs`, returning a `Vec<u8>` is necessary because the underlying key types (`Secp256k1VerifyingKey` and `Ed25519VerifyingKey`) provide their byte representations by value. Returning a `&[u8]` would involve borrowing from a temporary value, which is not feasible.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
crates/keys/src/signing_keys.rs (3)
7-7: LGTM for the p256 import.
The addition or modification to import p256 references for secp256r1 usage is consistent with the rest of the file.
40-40: Good switch to therandomconstructor.
UsingSecp256k1SigningKey::randomis the correct approach for k256-based key generation. Ensure integration tests cover this path for stable key generation.
93-95: Digest-based signing approach is valid.
Usingsign_digestaligns well with the new library’s API. Just confirm all sign/verify codepaths consistently use a hashed message.crates/keys/src/verifying_keys.rs (7)
11-11: Adoption of k256 verifying key.
Switching toSecp256k1VerifyingKeyfrom the k256 crate is consistent with the new secp256k1 logic.
64-64: Confirm compression settings.
to_sec1_bytes()can yield either compressed or uncompressed format depending on the library’s defaults. Confirm that usage here is consistent with your overall format choice (compressed vs. uncompressed).
72-72: DER output for secp256k1 key.
Generating DER withto_public_key_der()is valid. Ensure all consumers can handle the resulting ASN.1 structure properly.[approve]
83-83: Use offrom_sec1_bytes.
This is correct for p256/k256 style SEC1-encoded public keys. Confirm the function is robust to both compressed and uncompressed forms if needed.
127-129: Digest verification for secp256k1.
Usingverify_digestaligns well with thesign_digestapproach. This symmetry avoids confusion in hashing steps.
190-190: Cloning verifying key.
Invokingsk.verifying_key()followed by.to_owned()is reasonable to decouple lifetimes. No issues here.
241-242: Fallback to k256 if p256 decoding fails.
Because a 33/65-byte key could theoretically parse as either p256 or k256, ensure that collisions are unlikely or handled. In rare edge cases, a valid point on both curves could pass here.crates/keys/src/signatures.rs (4)
5-5: Switched tok256::ecdsa::Signature.
AdoptingSecp256k1Signaturefrom the k256 crate is proper, matching your key changes.
28-28: Check raw signature format.
sig.to_vec()returns the raw 64-byte R||S form. Verify this is the intended format for storage or transmission, as it differs from DER encoding.
36-36: DER serialization for secp256k1.
Confirm thatsig.to_der()produces a valid ASN.1 structure recognized across the ecosystem.
47-47: Switched tofrom_slicefor raw signature loading.
from_sliceexpects a 64-byte R||S signature. Ensure your external callers provide that format to avoid parsing errors.crates/keys/src/tests.rs (2)
5-5: LGTM! Import updated to use k256.The change from secp256k1 to k256 is correctly implemented.
143-143: LGTM! Key generation method updated.The change from
new()torandom()aligns with k256's API.crates/keys/Cargo.toml (1)
19-19: LGTM! Dependency updated to k256.The change correctly adds k256 as a workspace dependency, ensuring consistent versioning across the crate.
crates/zk/sp1-script/Cargo.toml (1)
33-33:❓ Verification inconclusive
Verify if secp256k1 patch is still needed.
Despite transitioning to k256, the secp256k1 patch is retained. This might be necessary for transitive dependencies, but it's worth verifying.
🏁 Script executed:
#!/bin/bash # Check for remaining secp256k1 dependencies cargo tree | grep secp256k1Length of output: 125
Manual Verification Required: secp256k1 Patch Necessity
The previous automated check couldn’t verify the secp256k1 dependency due to the unavailability of the cargo command in the sandbox environment. Please use the above shell script (or a manual search) to check for any references to "secp256k1" in the repository. This will confirm whether the patch retained in
crates/zk/sp1-script/Cargo.toml(line 33) is still needed for transitive dependency support despite transitioning to k256.
- File to check:
crates/zk/sp1-script/Cargo.toml(line 33)- Action Required: Verify that no transitive dependencies require
secp256k1. If unused, consider removing the patch; otherwise, document why it remains necessary.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for all references to "secp256k1" across the repository to determine its usage. rg "secp256k1"Length of output: 2631
Attention: Verify if the secp256k1 Patch Remains Necessary
The repository still contains several references to secp256k1 (e.g., in tests and key verification logic). Although there's been a transition toward k256, the custom secp256k1 patch in
crates/zk/sp1-script/Cargo.toml(line 33) might still be required to satisfy transitive dependencies or support legacy tests.
- Files to Review:
crates/zk/sp1-script/Cargo.toml(line 33): Confirm if the patched secp256k1 is still referenced downstream.crates/keys/src/verifying_keys.rsandcrates/keys/src/signing_keys.rs: Check usage of secp256k1 functions.- Test files in
crates/keys/src/tests.rsandcrates/tree/src/tests.rs: Verify that secp256k1-related tests are still valid.- Other references (e.g., in
justfileandcrates/node_types/prover/src/prover/tests.rs): Ensure consistency with the current cryptographic strategy.Please manually verify whether the secp256k1 patch is essential for supporting transitive dependencies or if the remaining usages can transition entirely to k256. If it’s no longer required, consider removing the patch along with any redundant tests; otherwise, document its necessity for future maintainability.
Cargo.toml (3)
107-107: LGTM! k256 dependency correctly configured.The k256 dependency is added with the necessary features (ecdsa, serde) for the codebase.
158-158: LGTM! k256 patch consistently versioned.The k256 patch is correctly configured and version-aligned with other elliptic curve patches.
1-1:❓ Verification inconclusive
Verify performance improvements.
The PR claims significant performance improvements, particularly for WebAssembly. Consider adding benchmarks to quantify the improvements.
🌐 Web query:
What are the performance differences between secp256k1 and k256 implementations, particularly for WebAssembly?💡 Result:
The performance differences between
secp256k1(C-based implementations) andk256(pure Rust) in WebAssembly (WASM) contexts depend on several factors:Key Performance Insights
Native vs. WASM Performance
- C-based
libsecp256k1(compiled to WASM):
- Achieves ~10x faster performance compared to JavaScript implementations[2][5].
- Remains ~3x slower than native Node.js bindings (e.g.,
secp256k1-node)[2].- Pure Rust
k256(compiled to WASM):
- Benchmarks show it is significantly slower than C-based WASM implementations[9]. For example:
k256signature verification is ~3x slower thanlibsecp256k1[4].WebAssembly Optimization Challenges
- Compiler optimizations for WASM (e.g., via LLVM) are less effective compared to native builds[3].
- Memory usage for WASM is ~2–5x higher than equivalent JavaScript code[3], impacting resource-constrained environments.
Practical Considerations
Factor secp256k1(C/WASM)k256(Rust/WASM)Speed Faster due to optimized C code Slower due to pure Rust implementation[9] Memory Usage Higher memory footprint[3][5] Lower overhead in Rust-native projects Portability Requires WASM toolchain (Emscripten/LLVM)[5] No external dependencies; no_stdsupport[6]Security Audited C code Not independently audited[8]
Use-Case Recommendations
- High-Performance Needs: Use C-based
libsecp256k1compiled to WASM for tasks like blockchain signing or encryption[2][5].- Rust Integration: Opt for
k256if portability and ease of integration in Rust projects outweigh raw speed[6][9].- Browser Environments: Both benefit from faster WASM parse/compile times compared to JavaScript[2][3], but performance varies across browsers (Firefox > Chrome > Edge)[3].
For example:
// Example: Using secp256k1-wasm in browsers import { sign } from 'secp256k1-wasm'; const signature = sign(privateKey, message);
Future Outlook
- WASM Performance Gains: As browser engines optimize WASM execution (e.g., JIT improvements), both implementations will see speedups[2][3].
- Rust Tooling: Ongoing optimizations in
k256may narrow the gap with C-based solutions[9].In summary, choose
libsecp256k1for speed-critical WASM applications andk256for Rust-centric projects prioritizing simplicity over maximum performance[2][6][9].Citations:
- 1: https://www.gpgfrontend.bktus.com/extra/algorithms-comparison/
- 2: https://blog.bitjson.com/just-released-webassembly-version-of-secp256k1-10x-faster-than-javascript/
- 3: https://weihang-wang.github.io/papers/imc21.pdf
- 4: https://github.com/tarcieri/rust-secp256k1-ecdsa-bench
- 5: https://www.npmjs.com/package/secp256k1-wasm
- 6: https://rustrepo.com/repo/RustCrypto-elliptic-curves
- 7: https://www.johndcook.com/blog/2018/08/21/a-tale-of-two-elliptic-curves/
- 8: https://docs.rs/secp256kfun_k256_backend
- 9: hyperledger-archives/ursa#163
- 10: https://docs.rs/k256/latest/k256/struct.Secp256k1.html
Performance Improvement Verification and Benchmarking Required
The PR claims significant performance improvements for WebAssembly. However, the current changes (e.g., in
Cargo.tomlwith[workspace.package]) do not include quantitative data. Benchmarks are needed to substantiate the performance claims.Based on recent findings:
- C-based
libsecp256k1compiled to WASM shows notable speedups (up to a ~10x improvement over JavaScript, though still about 3x slower than native bindings) compared to the pure Rustk256, which is approximately 3x slower in signature verification.- Memory usage and compiler optimizations can also differ substantially between these implementations.
Action Items:
- Add comprehensive benchmarks: Include tests, ideally using a benchmarking suite (e.g.,
criterion.rs), to measure and compare the actual performance improvements in your WASM environment.- Document observed metrics: Provide quantitative numbers to clearly demonstrate the claimed performance benefits over previous implementations.
jns-ps
changed the title
This PR dramatically reduces cycle count and enables k256 for wasm architecture
Before (without p256 precompile):
After adding p256 precompile:
After moving to k256:
Summary by CodeRabbit
Chores
secp256k1withk256for enhanced security and maintainability.Refactor
These improvements bolster the reliability and consistency of the cryptographic operations, providing a more robust security framework for users.