fix(hashing): improve byte array conversion methods #2279
Conversation
| // DANGEROUS: This will panic if slice length != $size | ||
| // Use from_slice() instead which safely checks the length and returns Result | ||
| impl<'a> From<&'a [u8]> for $name { | ||
| fn from(slc: &[u8]) -> Self { | ||
| assert_eq!(slc.len(), $size, "Input slice must be exactly {} bytes", $size); | ||
| let mut inner = [0u8; $size]; | ||
| inner[..].clone_from_slice(&slc[0..$size]); | ||
| inner.copy_from_slice(slc); | ||
| $name(inner) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is still used in a lot of places and we don't know if all the passed slices are checked for size or not, I already started removing this and depending on From<&'a [u8; $size]> instead. Will push a commit once done and will allow someone else to review this PR.
| let len = internal_id_hash.len(); | ||
| // Discuss: This truncates `internal_id_hash` to 32 bytes instead of using all 33 bytes (index + tx_hash). | ||
| // This is a limitation kept for backward compatibility. Changing to 33 bytes would | ||
| // alter the internal_id calculation, causing existing wallets to see duplicate transactions | ||
| // in their history. A proper migration would be needed to safely transition to using the full 33 bytes. | ||
| let internal_id_hash: [u8; 32] = match internal_id_hash | ||
| .get(..32) | ||
| .and_then(|slice| slice.try_into().ok()) | ||
| { | ||
| Some(hash) => hash, | ||
| None => { | ||
| log::debug!( | ||
| "Invalid internal_id_hash length for tx '{}' at index {}: expected 32 bytes, got {} bytes.", | ||
| tx_hash, | ||
| index, | ||
| len | ||
| ); | ||
| continue; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Maybe, we can use the correct calculation if it's a fresh start and do the otherwise if there are DB files already. After a year or so, we can remove the old/wrong approach from the code base. How does that sound?
| @@ -673,7 +673,7 @@ impl SwapOps for QtumCoin { | |||
| utxo_common::derive_htlc_key_pair(self.as_ref(), swap_unique_data) | |||
| } | |||
|
|
|||
| fn derive_htlc_pubkey(&self, swap_unique_data: &[u8]) -> Vec<u8> { | |||
| fn derive_htlc_pubkey(&self, swap_unique_data: &[u8]) -> [u8; 33] { | |||
There was a problem hiding this comment.
I think we should declare special types for all byte array types.
There was a problem hiding this comment.
But we already have all these H160/H256/H264/etc.. duplicated types from different crates. For instance we have 2 H160 types one for eth and one for utxo, should we add another kdf internal type? We then have to implement from for all these type, I think it complicates things. I would have gone for associated trait types but this code and others is used in legacy swap code and it would be a huge refactor to change those to associated types.
There was a problem hiding this comment.
If there are already too many types we should create our own type and wrap them inside it I think. We can also add validation functions and use them instead of manual checks like if x.len() != 32. Doing such checks and relying on [u8; N] all around the codebase makes it more complicated as it's harder to document and explain in my opinion.
#2275
Todo: