fix: correct BLS key derivation logic and improve test coverage #113
Conversation
WalkthroughReplaced HMAC-SHA512 BLS BIP32 derivation with a two-pass HMAC-SHA256 scheme; changed hardened/non-hardened input construction; private derivation now uses scalar addition, public derivation uses point addition; chain code creation uses Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Deriver as Derivation (derive_priv)
participant H1 as HMAC-SHA256 (pass 1)
participant H2 as HMAC-SHA256 (pass 2)
Caller->>Deriver: derive_priv(parent_sk, parent_cc, index, hardened?)
alt Hardened
Deriver->>Deriver: input := 0x00 || parent_sk || index
else Non-hardened
Deriver->>Deriver: input := parent_pk_bytes || index
end
Deriver->>H1: HMAC(parent_cc, input || 0x00)
H1-->>Deriver: tweak_bytes
Deriver->>H2: HMAC(parent_cc, input || 0x01)
H2-->>Deriver: chain_code_bytes
Deriver->>Deriver: tweak_sk := BlsSecretKey::from_bytes(tweak_bytes)
Deriver->>Deriver: child_sk := (parent_sk + tweak_sk) mod n
Deriver->>Caller: child_sk, ChainCode::from(chain_code_bytes)
sequenceDiagram
autonumber
actor Caller
participant Deriver as Derivation (derive_pub)
participant H1 as HMAC-SHA256 (pass 1)
participant H2 as HMAC-SHA256 (pass 2)
Caller->>Deriver: derive_pub(parent_pk, parent_cc, index)
note over Deriver: Non-hardened only
Deriver->>Deriver: input := parent_pk_bytes || index
Deriver->>H1: HMAC(parent_cc, input || 0x00)
H1-->>Deriver: tweak_bytes
Deriver->>H2: HMAC(parent_cc, input || 0x01)
H2-->>Deriver: chain_code_bytes
Deriver->>Deriver: tweak_sk := BlsSecretKey::from_bytes(tweak_bytes)
Deriver->>Deriver: tweak_pk := G * tweak_sk
Deriver->>Deriver: child_pk := parent_pk + tweak_pk
Deriver->>Caller: child_pk, ChainCode::from(chain_code_bytes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
key-wallet/src/derivation_bls_bip32.rs (3)
6-11: Fix doc: HMAC master key label in comments is inconsistent with code ("BLS12381 seed" vs "BLS HD seed").The top-of-file comment claims we use "BLS12381 seed", but the implementation uses "BLS HD seed". This mismatch can mislead users producing incompatible keys.
Apply this diff to make the docs match the implementation:
-//! - Uses "BLS12381 seed" as the HMAC key for master key generation +//! - Uses "BLS HD seed" as the HMAC key for master key generation
24-29: Gate serde imports behind the feature; current unconditionaluse serde::Deserializebreaks no-serde builds.Line 28 imports serde unconditionally, violating feature-gating and breaking builds when the
serdefeature is disabled.Apply this diff:
-#[cfg(feature = "serde")] -use serde; - -use serde::Deserialize; +#[cfg(feature = "serde")] +use serde::{self, Deserialize};
336-344: Avoid relying onPublicKey’s inner tuple-field for point addition; use the public API.Accessing
.0and constructingBlsPublicKey(derived_point)ties you to internal representation and may break with upstream changes. Prefer the library’s addition operator or helper API.Apply this diff if
Addis implemented:- let parent_point = self.public_key.0; - let tweak_point = tweak_pubkey.0; - - // Perform elliptic curve point addition - let derived_point = parent_point + tweak_point; - - // Create the new public key with the derived point - let derived_pubkey = BlsPublicKey(derived_point); + // Prefer high-level addition to avoid relying on internal fields + let derived_pubkey = self.public_key + tweak_pubkey;If
Addis not implemented onBlsPublicKey, add aPublicKey::add(&self, &other) -> Selfhelper in your wrapper layer rather than exposing.0.
🧹 Nitpick comments (5)
key-wallet/src/derivation_bls_bip32.rs (5)
152-166: Unify HMAC input encoding for non-hardened derivation (priv vs. pub path).Private path uses
public_key_bytes()(fixed 48 array) while public path usespublic_key.to_bytes()(full encoding). These MUST be identical to ensure cross-derivation consistency and long-term compatibility with external implementations. Use the same serialization in both places.Apply this diff to align the private path with the public path:
- let public_key_bytes = self.public_key_bytes(); - input_data.extend_from_slice(&public_key_bytes); + let public_key_bytes = self.public_key().to_bytes(); + input_data.extend_from_slice(&public_key_bytes);Optionally, make
public_key_bytes()delegate toself.public_key().to_bytes()or remove it to avoid dual encodings.Also applies to: 298-303
171-183: Minor readability: avoid in-place index mutation when flipping the suffix.Using
input_with_suffix[input_data.len()] = 1;is correct but brittle. Extending with a new byte is clearer and avoids relying on the exact index.Apply these diffs:
- let mut input_with_suffix = input_data.clone(); - input_with_suffix.push(0); + let mut input_with_suffix = input_data.clone(); + input_with_suffix.push(0); // ... - input_with_suffix[input_data.len()] = 1; + input_with_suffix.pop(); + input_with_suffix.push(1);Same change for the public derivation block.
Also applies to: 308-319
217-223: Potential panic and unnecessary copy inpublic_key_bytes().
array.copy_from_slice(&bytes[..48.min(bytes.len())])will panic ifbytes.len() < 48(dest is always 48). Ifto_bytes()is guaranteed to be exactly 48 forBls12381G2Impl, you can copy directly; otherwise return a Vec or validate length.Proposed simplification if 48-byte guarantee holds:
- pub fn public_key_bytes(&self) -> [u8; 48] { - let bytes = self.public_key().to_bytes(); - let mut array = [0u8; 48]; - array.copy_from_slice(&bytes[..48.min(bytes.len())]); - array - } + pub fn public_key_bytes(&self) -> [u8; 48] { + let bytes = self.public_key().to_bytes(); + // If this assert can fail, prefer returning Vec<u8> instead. + debug_assert_eq!(bytes.len(), 48, "Unexpected pubkey length"); + bytes.as_slice().try_into().expect("48-byte pubkey") + }Please confirm the exact length returned by
to_bytes()forBls12381G2Impl.
101-147: Optional hardening: zeroize sensitive buffers (seed_with_suffix).
seed_with_suffixholds material used to derive the master key. Consider clearing it after use (and any temporary copies) using thezeroizecrate. This is defense-in-depth for cryptographic code.Example:
+use zeroize::Zeroize; ... let chain_code_bytes = hmac_result2.as_byte_array(); Ok(ExtendedBLSPrivKey { network, depth: 0, parent_fingerprint: Default::default(), child_number: ChildNumber::from_normal_idx(0).unwrap(), private_key, chain_code: ChainCode::from(*chain_code_bytes), }) } + // Before returning you can zeroize the temp buffer: + // seed_with_suffix.zeroize();If introducing a new dependency is undesirable for now, consider wrapping with
Zeroizing<Vec<u8>>behind a feature.
32-49: Error handling style: considerthiserrorfor consistency with repo guidelines.Current enum + Display works, but the guidelines request
thiserror. Migrating aligns diagnostics and reduces boilerplate.Example:
-#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)] pub enum Error { - /// Invalid derivation path string - InvalidDerivationPath, + #[error("Invalid derivation path")] + InvalidDerivationPath, ... - BLSError(String), + #[error("BLS error: {0}")] + BLSError(String), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
key-wallet/src/derivation_bls_bip32.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with the thiserror crate and propagate errors appropriately
Use the tokio runtime for async operations
Adhere to MSRV: code must compile on Rust 1.89 (avoid newer language/features)
Keep code formatted with rustfmt (cargo fmt)
Keep the codebase clippy-clean (clippy with -D warnings)
Files:
key-wallet/src/derivation_bls_bip32.rs
{**/*.rs,**/Cargo.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use feature flags and conditional compilation for optional features
Files:
key-wallet/src/derivation_bls_bip32.rs
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}
📄 CodeRabbit inference engine (CLAUDE.md)
{key-wallet/**/*.rs,key-wallet-ffi/**/*.rs,swift-dash-core-sdk/**/*.swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet/src/derivation_bls_bip32.rs
🧠 Learnings (1)
📚 Learning: 2025-02-25T06:13:52.858Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.
Applied to files:
key-wallet/src/derivation_bls_bip32.rs
🧬 Code graph analysis (1)
key-wallet/src/derivation_bls_bip32.rs (3)
key-wallet/src/bip32.rs (7)
child(1305-1309)master(1294-1296)new_master(1468-1481)from_normal_idx(600-608)from_hardened_idx(614-622)fingerprint(1727-1729)fingerprint(2021-2023)key-wallet/src/derivation_slip10.rs (3)
new_master(50-75)fingerprint(133-139)fingerprint(283-288)key-wallet/src/wallet/root_extended_keys.rs (1)
new_master(31-60)
🪛 GitHub Actions: Continuous integration
key-wallet/src/derivation_bls_bip32.rs
[error] 184-1179: cargo fmt detected formatting differences in key-wallet/src/derivation_bls_bip32.rs. Run 'cargo fmt' to apply formatting changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
🔇 Additional comments (8)
key-wallet/src/derivation_bls_bip32.rs (8)
862-887: Tests: long derivation path coverage looks good.Covers hardened + non-hardened mix and depth/chain-code updates; nice parity checks.
889-948: Tests: serde and bincode roundtrips are thorough.Good separation via features; validates all metadata and key bytes. Keep it.
950-985: Tests: cross-derivation (priv vs pub) sanity checks.This ensures encoding consistency of HMAC inputs; will catch future regressions after unifying serialization.
987-1027: Tests: C++ compatibility vectors.Excellent for ensuring interoperability with the reference implementation.
1029-1079: Tests: legacy HD compatibility.Good end-to-end assertions (public keys and chain codes, depth, child number).
1082-1147: Tests: extended unhardened derivation scenarios.Confidence-inspiring coverage across different seeds and multi-level derivations.
1149-1194: Tests: hardened vs. unhardened comparison.The determinism and inequality assertions are spot-on.
1-1: Manual Formatting Verification RequiredWe attempted to run
cargo fmtin the sandbox but encountered an environment limitation (no /proc/self/exe available). Please verify formatting locally:# Ensure all Rust files are formatted cargo fmt --all # Confirm no formatting diffs remain cargo fmt --all -- --checkOnce formatting is clean, commit and push the changes so CI will pass.
| BlsSecretKey::<Bls12381G2Impl>::from_be_bytes(key_bytes) | ||
| .into_option() | ||
| .ok_or(Error::InvalidPrivateKey)?; | ||
| // Add keys together - BLS library handles the modular arithmetic | ||
| // For now, we'll regenerate from combined bytes (simplified) | ||
| let parent_bytes = self.private_key.to_be_bytes(); | ||
| let tweak_bytes = tweak_key.to_be_bytes(); | ||
| let mut combined = [0u8; 32]; | ||
| let mut carry = 0u16; | ||
| for i in (0..32).rev() { | ||
| let sum = parent_bytes[i] as u16 + tweak_bytes[i] as u16 + carry; | ||
| combined[i] = (sum & 0xff) as u8; | ||
| carry = sum >> 8; | ||
| } | ||
| BlsSecretKey::<Bls12381G2Impl>::from_be_bytes(&combined) | ||
| .into_option() | ||
| .ok_or(Error::InvalidPrivateKey)? | ||
|
|
||
| // Perform scalar addition in the BLS12-381 field | ||
| // The SecretKey struct has a public field (0) containing the scalar | ||
| // We add the scalars and create a new SecretKey from the result | ||
| let parent_scalar = self.private_key.0; | ||
| let tweak_scalar = tweak_key.0; | ||
| let derived_scalar = parent_scalar + tweak_scalar; | ||
|
|
||
| BlsSecretKey::<Bls12381G2Impl>(derived_scalar) | ||
| }; |
There was a problem hiding this comment.
Do not construct SecretKey via tuple-field; validate the sum and use safe constructors.
Directly accessing self.private_key.0 and constructing BlsSecretKey(derived_scalar) bypasses invariants (e.g., non-zero scalar). If parent_scalar + tweak_scalar == 0 mod r, you silently create an invalid private key. Per BIP32-like schemes, such results must be rejected.
Apply a safer construction that re-validates the derived scalar and rejects zero. Example (adjust to actual API):
- let parent_scalar = self.private_key.0;
- let tweak_scalar = tweak_key.0;
- let derived_scalar = parent_scalar + tweak_scalar;
-
- BlsSecretKey::<Bls12381G2Impl>(derived_scalar)
+ let parent_scalar = self.private_key.0;
+ let tweak_scalar = tweak_key.0;
+ let derived_scalar = parent_scalar + tweak_scalar;
+
+ // Reject invalid (zero) result and use a safe constructor
+ let derived_bytes = derived_scalar.to_be_bytes();
+ BlsSecretKey::<Bls12381G2Impl>::from_be_bytes(&derived_bytes)
+ .into_option()
+ .ok_or(Error::InvalidPrivateKey)?If the Scalar type doesn’t expose to_be_bytes()/is_zero(), use a library-provided SecretKey addition API if available (preferred), or add a helper that performs the addition via safe constructors only. The key point: avoid tuple-field construction and validate non-zero.
…tation - Add long derivation path test (m/(2^31+5)/0/0/(2^31+56)/70/4) - Add serialization roundtrip tests for ExtendedBLSPrivKey and ExtendedBLSPubKey - Add exact C++ test vectors for compatibility verification - Add legacy HD compatibility tests - Add extended unhardened derivation tests - Refactor hardened index creation to use from_hardened_idx() method - Ensure full test parity with C++ bls-signatures library All 16 BLS BIP32 tests passing with complete coverage
Summary by CodeRabbit
Refactor
Tests
Chores