fix(hashing): improve byte array conversion methods #2279
Conversation
| 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?
There was a problem hiding this comment.
Sounds fine to me.
we can remove the old/wrong approach from the code base.
We can even delete the old entries and resync history instead. I think this should be handled in another PR and to leave this as is for now since this PR purpose is to fix the security problems by removing potential panics. I will open an issue for this.
| @@ -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.
There was a problem hiding this comment.
I agree on creating our own types, but shouldn't be done in this PR to not make it grow more as this PR fixes the panic issue only. No need to open an issue as associated trait types or our own types should be part of the whole project refactor. I can mention this here #1247
#2275
Todo: