Avoid possible truncation of higher bits #619
Conversation
| (*Hasher::default().chain(txid).chain([idx as u8]).finalize()).into() | ||
| (*Hasher::default() | ||
| .chain(txid) | ||
| .chain(idx.to_bytes()) |
There was a problem hiding this comment.
If we are okay with supporting more than 255 receipts, then we also need to update the specification
There was a problem hiding this comment.
I think this was settled yesterday and we do want to support U16::MAX receipts. Would a PR changing the linked paragraph be enough? I recall there was some discussion about serialization as well.
There was a problem hiding this comment.
We need to update the paragraph and mention that we need to use canonical serialization for integers.
We have a separate issue to add a u16::max limit #620
…is impossible to be more than `u8`
|
|
||
| #[test] | ||
| fn test_msb_for_bytes_1() { | ||
| const NUM_BITS: usize = size_of::<Bytes1>() * 8; |
There was a problem hiding this comment.
I like the size_of approach as it's more obvious to the code reader how this value is constructed. Maybe we should split this into two constants like NUM_BYTES vs NUM_BITS?
| @@ -5,11 +5,11 @@ pub enum Bit { | |||
| } | |||
|
|
|||
| trait GetBit { | |||
| fn get_bit(&self, bit_index: usize) -> Option<Bit>; | |||
| fn get_bit(&self, bit_index: u32) -> Option<Bit>; | |||
There was a problem hiding this comment.
Are these changes to fuel-merkle necessary? It feels like we're changing library logic to fit business logic. A library should promise correctness under all targets it supports, including all 32 bit and 64 bit targets. If it has internal problems with truncation, that should be fixed in the library. But this change appears to be changing the library interface to accommodate changes to external libraries.
There was a problem hiding this comment.
It is a private trait, so it is an internal detail of how Merkle tree works=)
Closes #1261 Awaits reelasing of the FuelLabs/fuel-vm#619. --------- Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
Closes FuelLabs/fuel-core#1261 Awaits reelasing of the FuelLabs/fuel-vm#619. --------- Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
Closes #570