Add DIP 14 derivation and DIP 9 known derivation Paths. #39
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PSBT
participant BIP32
participant DIP9
User->>PSBT: Initiate PSBT workflow
PSBT->>BIP32: Derive keys using updated private key
BIP32->>DIP9: Access derivation paths and types
DIP9-->>BIP32: Provide derivation paths
BIP32-->>PSBT: Return derived keys
PSBT-->>User: Complete PSBT workflow
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (10)
dash/Cargo.toml (1)
60-60: LGTM: Addition of bitflags dependency.The addition of the
bitflagscrate (version 2.6.0) is appropriate. This widely-used library will provide efficient bit flag manipulation capabilities to the project.Consider adding a comment in the code where
bitflagsis first used to explain its purpose in the context of this project.dash/src/lib.rs (1)
105-105: LGTM. Consider visibility and documentation for the newdip9module.The addition of the
dip9module is consistent with the file structure and aligns with the PR objectives. However, consider the following suggestions:
- Evaluate whether this module should be public (
pub mod dip9;) to allow access from outside the crate.- Add a brief doc comment explaining the purpose of this module, e.g.:
/// Module implementing DIP 9 and DIP 14 derivation paths. mod dip9;- If appropriate, consider re-exporting key types or functions from this module at the crate level for easier access.
dash/tests/psbt.rs (2)
128-128: LGTM! Consider adding a comment explaining the key change.The update to the extended private key string looks good. This change likely aligns the test with updated specifications or new test vectors.
Consider adding a brief comment explaining why the extended private key was changed, to provide context for future maintainers.
🧰 Tools
🪛 Gitleaks
128-128: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
128-128: Add a comment to prevent false positives in static analysis.The extended private key string is being flagged as a potential API key by static analysis tools. While this is a false positive, adding a comment can help prevent future confusion.
Consider adding a comment like the following above the
extended_private_keydeclaration:+ // This is a test vector for an extended private key, not an API key let extended_private_key = "tprv8ZgxMBicQKsPd9TeAdPADNnSyH9SSUUbTVeFszDE23Ki6TBB5nCefAdHkK8Fm3qMQR6sHwA56zqRmKmxnHk37JkiFzvncDqoKmPWubu7hDF";🧰 Tools
🪛 Gitleaks
128-128: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
dash/src/dip9.rs (4)
1-19: Consider adding documentation comments toDerivationPathReferenceenumAdding documentation comments to the
DerivationPathReferenceenum and its variants would improve code readability and help users understand the purpose of each variant.
27-47: Consider adding documentation comments toDerivationPathTypestruct and flagsProviding documentation comments for the
DerivationPathTypestruct and its associated flags would enhance clarity and assist users in understanding the different path types and their usage.
49-55: Consider adding documentation toIndexConstPathstructAdding documentation comments to the
IndexConstPathstruct would help explain its purpose and how it should be used, benefiting other developers who interact with this code.
1-206: Consider adding unit tests for new derivation paths and methodsIncluding unit tests for the newly added derivation paths and associated methods would help ensure their correctness and prevent future regressions.
dash/src/bip32.rs (2)
276-281: Consistent formatting infmt::Displayfor 256-bit indices.In the
fmt::Displayimplementation forChildNumber, ensure that the formatting of 256-bit indices is consistent and unambiguous. For example, always include the'orhsuffix for hardened keys, and consider using a consistent prefix like0xfor hexadecimal representations.
560-562: Consider providing more descriptive error variants.The
Error::Hex(hashesHex::Error)variant is added to handle hex decoding errors. For better error reporting, consider wrapping the underlying error with additional context or creating a specific error variant for child number parsing errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
- dash/Cargo.toml (1 hunks)
- dash/src/bip32.rs (20 hunks)
- dash/src/dip9.rs (1 hunks)
- dash/src/lib.rs (1 hunks)
- dash/tests/psbt.rs (2 hunks)
🧰 Additional context used
🪛 Gitleaks
dash/tests/psbt.rs
128-128: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
dash/src/dip9.rs (2)
112-115: Verify ifFEATURE_PURPOSE_IDENTITIES_SUBFEATURE_AUTHENTICATIONshould have a value of0The constant
FEATURE_PURPOSE_IDENTITIES_SUBFEATURE_AUTHENTICATIONis set to0. Please confirm if assigning0to this sub-feature index is intentional. In some cases, using0might have special implications or might be used to represent a default or uninitialized state in derivation paths.
117-206: Ensure consistency in derivation path constantsThe derivation path constants for mainnet and testnet are defined with their respective coin types and sub-feature indices. Please verify that all paths are correctly defined and that the indices correspond to the intended hierarchical levels as per the standards.
dash/src/bip32.rs (1)
1046-1051: Verify version bytes inencode_256forExtendedPubKey.The version bytes used in the
encode_256method forExtendedPubKeyare[0x0E, 0xEC, 0xEF, 0xC5]forNetwork::Dashand[0x0E, 0xED, 0x27, 0x0B]forNetwork::Testnet. Ensure that these version bytes are correct and align with the DIP-14 specifications to prevent compatibility issues.You can verify the correctness of the version bytes by checking the DIP-14 documentation or official Dash sources.
| @@ -34,6 +34,7 @@ macro_rules! hex_psbt { | |||
| }; | |||
| } | |||
|
|
|||
| #[ignore] | |||
There was a problem hiding this comment.
💡 Codebase verification
#[ignore] attribute is still present in dash/tests/psbt.rs.
- Remove the
#[ignore]attribute to ensure thebip174_psbt_workflowtest runs as part of the test suite.
🔗 Analysis chain
Consider removing #[ignore] and updating the entire test.
The change in the extended private key will affect the entire PSBT workflow test. Since this test is currently ignored, there's a risk that the changes haven't been fully validated against the new key.
- Please review and update all parts of the
bip174_psbt_workflowtest that depend on this key. - Consider removing the
#[ignore]attribute to ensure this test runs regularly and catches any regressions.
To help verify the impact, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for occurrences of the old and new extended private key in the test file
OLD_KEY="tprv8ZgxMBicQKsPeUbMS6kswJc11zgVEXUnUZuGo3bF6bBrAg1ieFfUdPc9UHqbD5HcXizThrcKike1c4z6xHrz6MWGwy8L6YKVbgJMeQHdWDp"
NEW_KEY="tprv8ZgxMBicQKsPd9TeAdPADNnSyH9SSUUbTVeFszDE23Ki6TBB5nCefAdHkK8Fm3qMQR6sHwA56zqRmKmxnHk37JkiFzvncDqoKmPWubu7hDF"
echo "Checking for old key occurrences:"
rg "$OLD_KEY" dash/tests/psbt.rs
echo "Checking for new key occurrences:"
rg "$NEW_KEY" dash/tests/psbt.rs
echo "Checking for #[ignore] attribute:"
rg "#\[ignore\]" dash/tests/psbt.rs
Length of output: 940
| let secp = Secp256k1::new(); | ||
| let sk = ExtendedPrivKey::new_master(network, seed)?; | ||
| let path = self.append_path(add_derivation_path); | ||
| sk.derive_priv(&secp, &path) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse Secp256k1 context for improved performance
The methods derive_priv_for_master_seed, derive_pub_for_master_seed, and derive_pub_for_master_extended_public_key each create a new Secp256k1 context instance using Secp256k1::new(). Since creating a Secp256k1 context is relatively inexpensive but can be reused, consider passing a reference to a single Secp256k1 context or using a static instance to improve performance.
Also applies to: 91-94, 101-104
| ChildNumber::Normal256 { .. } => u32::MAX, | ||
| ChildNumber::Hardened256 { .. } => u32::MAX, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with From<ChildNumber> implementation for 256-bit indices.
In the implementation of From<ChildNumber> for u32, mapping ChildNumber::Normal256 and ChildNumber::Hardened256 to u32::MAX could lead to unintended behavior or silent errors, as u32::MAX is a valid 32-bit child number. This could cause collisions or misrepresentations when converting ChildNumber variants to u32. Consider returning a Result<u32, Error> or an Option<u32> to handle cases where a 256-bit index cannot be represented as a u32.
| Normal { .. } | ChildNumber::Normal256 { .. } => { | ||
| let pk2 = pk.ckd_pub(secp, num).unwrap(); | ||
| pk = ExtendedPubKey::from_priv(secp, &sk); | ||
| assert_eq!(pk, pk2); | ||
| } | ||
| Hardened { .. } => { | ||
| Hardened { .. } | ChildNumber::Hardened256 { .. } => { | ||
| assert_eq!(pk.ckd_pub(secp, num), Err(Error::CannotDeriveFromHardenedKey)); | ||
| pk = ExtendedPubKey::from_priv(secp, &sk); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for 256-bit child number variants.
In the test suite, consider adding more test cases specifically targeting the new Normal256 and Hardened256 variants. This includes testing edge cases like maximum and minimum possible values, incrementing behavior, serialization/deserialization, and ensuring that the new variants interact correctly with existing functionality.
| ChildNumber::Normal256 { mut index } => { | ||
| // Increment the 256-bit big-endian number represented by index | ||
| let mut carry = 1u8; | ||
| for byte in index.iter_mut().rev() { | ||
| let (new_byte, overflow) = byte.overflowing_add(carry); | ||
| *byte = new_byte; | ||
| carry = if overflow { 1 } else { 0 }; | ||
| if carry == 0 { | ||
| break; | ||
| } | ||
| } | ||
| if carry != 0 { | ||
| // Overflow occurred | ||
| return Err(Error::InvalidChildNumber(0)); // Or define a suitable error | ||
| } | ||
| Ok(ChildNumber::Normal256 { index }) | ||
| } | ||
| ChildNumber::Hardened256 { mut index } => { | ||
| // Increment the 256-bit big-endian number represented by index | ||
| let mut carry = 1u8; | ||
| for byte in index.iter_mut().rev() { | ||
| let (new_byte, overflow) = byte.overflowing_add(carry); | ||
| *byte = new_byte; | ||
| carry = if overflow { 1 } else { 0 }; | ||
| if carry == 0 { | ||
| break; | ||
| } | ||
| } | ||
| if carry != 0 { | ||
| // Overflow occurred | ||
| return Err(Error::InvalidChildNumber(0)); // Or define a suitable error | ||
| } | ||
| Ok(ChildNumber::Hardened256 { index }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the increment logic for 256-bit indices.
The current implementation of the increment method for 256-bit indices manually handles carry-over using byte-wise addition. This can be optimized for readability and performance by utilizing existing crates like uint or num-bigint that offer arbitrary-sized integer arithmetic.
Consider refactoring the increment logic as follows:
use num_bigint::BigUint;
use num_traits::One;
impl ChildNumber {
// ...
ChildNumber::Normal256 { index } => {
let value = BigUint::from_bytes_be(&index);
let incremented = value + BigUint::one();
let new_index = incremented.to_bytes_be();
if new_index.len() > 32 {
return Err(Error::InvalidChildNumber(0)); // Overflow occurred
}
let mut index_bytes = [0u8; 32];
index_bytes[32 - new_index.len()..].copy_from_slice(&new_index);
Ok(ChildNumber::Normal256 { index: index_bytes })
}
// Similar changes for Hardened256
}| _ => panic!("Invalid child number for 256-bit format"), | ||
| }; | ||
| ret[9] = hardening_byte; | ||
|
|
||
| // Child number (32 bytes) | ||
| let child_number_bytes = match self.child_number { | ||
| ChildNumber::Normal256 { index } | ChildNumber::Hardened256 { index } => index, | ||
| _ => panic!("Invalid child number for 256-bit format"), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using panic! in library code; handle errors gracefully.
Using panic! in the encode_256 method for invalid child numbers can cause the application to terminate unexpectedly. Instead, consider returning an Error to allow the calling code to handle the situation gracefully.
Apply this diff to handle errors without panicking:
fn encode_256(&self) -> Result<[u8; 107], Error> {
let mut ret = [0u8; 107];
// Version bytes
let version: [u8; 4] = match self.network {
Network::Dash => [0x0E, 0xEC, 0xF0, 0x2E],
Network::Testnet | Network::Devnet | Network::Regtest => [0x0E, 0xED, 0x27, 0x74],
};
ret[0..4].copy_from_slice(&version);
// Depth
ret[4] = self.depth;
// Parent fingerprint
ret[5..9].copy_from_slice(&self.parent_fingerprint[..]);
// Hardening byte
let hardening_byte = match self.child_number {
ChildNumber::Normal256 { .. } => 0x00,
ChildNumber::Hardened256 { .. } => 0x01,
- _ => panic!("Invalid child number for 256-bit format"),
+ _ => return Err(Error::InvalidChildNumberFormat),
};
ret[9] = hardening_byte;
// Child number (32 bytes)
let child_number_bytes = match self.child_number {
ChildNumber::Normal256 { index } | ChildNumber::Hardened256 { index } => index,
- _ => panic!("Invalid child number for 256-bit format"),
+ _ => return Err(Error::InvalidChildNumberFormat),
};
ret[10..42].copy_from_slice(&child_number_bytes);
// Chain code (32 bytes)
ret[42..74].copy_from_slice(&self.chain_code[..]);
// Key data (33 bytes)
ret[74] = 0x00; // Padding for private key
ret[75..107].copy_from_slice(&self.private_key[..]);
- ret
+ Ok(ret)
}📝 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.
| _ => panic!("Invalid child number for 256-bit format"), | |
| }; | |
| ret[9] = hardening_byte; | |
| // Child number (32 bytes) | |
| let child_number_bytes = match self.child_number { | |
| ChildNumber::Normal256 { index } | ChildNumber::Hardened256 { index } => index, | |
| _ => panic!("Invalid child number for 256-bit format"), | |
| }; | |
| _ => return Err(Error::InvalidChildNumberFormat), | |
| }; | |
| ret[9] = hardening_byte; | |
| // Child number (32 bytes) | |
| let child_number_bytes = match self.child_number { | |
| ChildNumber::Normal256 { index } | ChildNumber::Hardened256 { index } => index, | |
| _ => return Err(Error::InvalidChildNumberFormat), | |
| }; |
| let is_hardened = inp.ends_with('\'') || inp.ends_with('h'); | ||
| let index_str = if is_hardened { &inp[..inp.len() - 1] } else { inp }; | ||
|
|
||
| if index_str.starts_with("0x") || index_str.starts_with("0X") { | ||
| // Parse as a 256-bit hex number | ||
| let hex_str = &index_str[2..]; | ||
| let hex_bytes = hex::decode(hex_str).map_err(|_| Error::InvalidChildNumberFormat)?; | ||
| if hex_bytes.len() != 32 { | ||
| return Err(Error::InvalidChildNumberFormat); | ||
| } | ||
| let mut index_bytes = [0u8; 32]; | ||
| index_bytes[32 - hex_bytes.len()..].copy_from_slice(&hex_bytes); | ||
| if is_hardened { | ||
| Ok(ChildNumber::Hardened256 { index: index_bytes }) | ||
| } else { | ||
| Ok(ChildNumber::Normal256 { index: index_bytes }) | ||
| } | ||
| } else { | ||
| ChildNumber::from_normal_idx(inp.parse().map_err(|_| Error::InvalidChildNumberFormat)?)? | ||
| }) | ||
| // Parse as a u32 number | ||
| let index = index_str.parse::<u32>().map_err(|_| Error::InvalidChildNumberFormat)?; | ||
| if is_hardened { | ||
| ChildNumber::from_hardened_idx(index) | ||
| } else { | ||
| ChildNumber::from_normal_idx(index) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate 256-bit index length in FromStr implementation.
When parsing a hexadecimal string for a 256-bit index in the FromStr implementation of ChildNumber, the code does not enforce that the hex string represents exactly 64 hexadecimal characters (32 bytes). If the input is shorter, the index is left-padded with zeros, which might not be intended and could lead to incorrect child numbers. It's advisable to enforce the exact length requirement for 256-bit indices and return an error if the input length is incorrect.
Apply this diff to enforce the length check:
if index_str.starts_with("0x") || index_str.starts_with("0X") {
// Parse as a 256-bit hex number
let hex_str = &index_str[2..];
+ if hex_str.len() != 64 {
+ return Err(Error::InvalidChildNumberFormat);
+ }
let hex_bytes = hex::decode(hex_str).map_err(|_| Error::InvalidChildNumberFormat)?;
let mut index_bytes = [0u8; 32];
index_bytes.copy_from_slice(&hex_bytes);
if is_hardened {
Ok(ChildNumber::Hardened256 { index: index_bytes })
} else {
Ok(ChildNumber::Normal256 { index: index_bytes })
}
}📝 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.
| let is_hardened = inp.ends_with('\'') || inp.ends_with('h'); | |
| let index_str = if is_hardened { &inp[..inp.len() - 1] } else { inp }; | |
| if index_str.starts_with("0x") || index_str.starts_with("0X") { | |
| // Parse as a 256-bit hex number | |
| let hex_str = &index_str[2..]; | |
| let hex_bytes = hex::decode(hex_str).map_err(|_| Error::InvalidChildNumberFormat)?; | |
| if hex_bytes.len() != 32 { | |
| return Err(Error::InvalidChildNumberFormat); | |
| } | |
| let mut index_bytes = [0u8; 32]; | |
| index_bytes[32 - hex_bytes.len()..].copy_from_slice(&hex_bytes); | |
| if is_hardened { | |
| Ok(ChildNumber::Hardened256 { index: index_bytes }) | |
| } else { | |
| Ok(ChildNumber::Normal256 { index: index_bytes }) | |
| } | |
| } else { | |
| ChildNumber::from_normal_idx(inp.parse().map_err(|_| Error::InvalidChildNumberFormat)?)? | |
| }) | |
| // Parse as a u32 number | |
| let index = index_str.parse::<u32>().map_err(|_| Error::InvalidChildNumberFormat)?; | |
| if is_hardened { | |
| ChildNumber::from_hardened_idx(index) | |
| } else { | |
| ChildNumber::from_normal_idx(index) | |
| } | |
| } | |
| let is_hardened = inp.ends_with('\'') || inp.ends_with('h'); | |
| let index_str = if is_hardened { &inp[..inp.len() - 1] } else { inp }; | |
| if index_str.starts_with("0x") || index_str.starts_with("0X") { | |
| // Parse as a 256-bit hex number | |
| let hex_str = &index_str[2..]; | |
| if hex_str.len() != 64 { | |
| return Err(Error::InvalidChildNumberFormat); | |
| } | |
| let hex_bytes = hex::decode(hex_str).map_err(|_| Error::InvalidChildNumberFormat)?; | |
| let mut index_bytes = [0u8; 32]; | |
| index_bytes.copy_from_slice(&hex_bytes); | |
| if is_hardened { | |
| Ok(ChildNumber::Hardened256 { index: index_bytes }) | |
| } else { | |
| Ok(ChildNumber::Normal256 { index: index_bytes }) | |
| } | |
| } else { | |
| // Parse as a u32 number | |
| let index = index_str.parse::<u32>().map_err(|_| Error::InvalidChildNumberFormat)?; | |
| if is_hardened { | |
| ChildNumber::from_hardened_idx(index) | |
| } else { | |
| ChildNumber::from_normal_idx(index) | |
| } | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Tests