Fix off-by-one. #243
Fix off-by-one. #243
Conversation
There was a problem hiding this comment.
Nice catch!
I think in parity-ethereum we deny hex string of odd length before deserializing them.
The new behavior is to essentially pad it with leading 0 if it has an odd length, so e.g. 0x123 is treated as 0x0123. This seems reasonable to me, but should be documented somewhere.
|
@ordian Indeed Parity Ethereum is following the jsonrpc spec from here: https://github.com/ethereum/wiki/wiki/JSON-RPC#hex-value-encoding and it clearly states what values are valid and what not. |
| @@ -105,7 +105,7 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> where | |||
|
|
|||
| let bytes_len = v.len() - 2; | |||
| let mut modulus = bytes_len % 2; | |||
| let mut bytes = vec![0u8; bytes_len / 2 + 1]; | |||
| let mut bytes = vec![0u8; (bytes_len + 1) / 2]; | |||
There was a problem hiding this comment.
why bother with this complex logic?
Isn't better to use Vec::new() / Vec::with_capacity() and Vec::push?
There was a problem hiding this comment.
Well, it's a performance optimization (hopefuly measureable), which is justified, since this code might be on a hot path.
Also this logic is not that complex imho, seems it was just untested properly.
There was a problem hiding this comment.
I for one kind of agree with Niklas here: it's a bit odd and I'm surprised to learn it's faster to init a vec with vec![0u8; 123] than Vec::with_capacity(123).
(No action needed here, I'm just curious)
There was a problem hiding this comment.
I'm about to add benches for Bytes to be continued :P
There was a problem hiding this comment.
Wow, thanks for making me realize this is actually a vec. I was pretty sure that we just allocate an array here (but obviously we don't cause it's not a constant size)
I'm really curious about the performance indeed, as just using push instead of direct access seems way less error prone, and with_capacity should ensure that there are no re-allocations.
There was a problem hiding this comment.
I really hope there's no difference in performance! :)
No description provided.