Skip to content

Conversation

@etan-k
Copy link

@etan-k etan-k commented Jul 25, 2021

When converting hex strings into fixed size byte arrays, the function
hexToPaddedByteArray is commonly used to deal with the input data
being shorter than the target buffer. However, the opposite case of the
input data having extra leading zeroes leads to an error.

This patch allows leading zeroes in hexToPaddedByteArray, ignoring
them when the input string is longer than the destination array.

When converting hex strings into fixed size byte arrays, the function
`hexToPaddedByteArray` is commonly used to deal with the input data
being shorter than the target buffer. However, the opposite case of the
input data having extra leading zeroes leads to an error.

This patch allows leading zeroes in `hexToPaddedByteArray`, ignoring
them when the input string is longer than the destination array.
@etan-k
Copy link
Author

etan-k commented Jul 25, 2021

I noticed this limitation when using nim-blscurve to import a private key from the Ethereum 2.0 spec, i.e., https://github.com/ethereum/eth2.0-specs/blob/dev/tests/generators/bls/main.py#L45-L51

nim-blscurve's BLST fromHex functions internally use hexToPaddedByteArray and do not allow the representation from the spec with extra leading zeroes. This is in contrast with other libraries such as the Milagro crypto backend.

@etan-k etan-k marked this pull request as ready for review July 25, 2021 19:32
@zah
Copy link
Contributor

zah commented Jun 17, 2022

So, the intention here is that you want to be able to decode a string such as "0x0000ffff" into a 3-element array and get the result [0, 255, 255]? That would be quite surprising semantics for me. Can you tell us more about the specific use case where this seems the most appropriate handling?

@etan-k
Copy link
Author

etan-k commented Jun 17, 2022

Anytime a number is imported. The semantics are same regardless of number of leading zeroes. The function already does the opposite of padding with extra zeroes, e.g., passing "0xffff" to the 3-element array would also result in [0, 255, 255].

Example use case was given above, the Eth2 sample private keys contain a lot of extra zeroes, which does not make them invalid as it is still representing the same number, but putting them as is into the hexToPaddedByteArray function leads to errors.

See https://github.com/ethereum/consensus-specs/blob/8cc008d11c79b6a7f4e41e4d36a0b8ac7d3f0c67/tests/generators/bls/main.py#L47-L53

@zah
Copy link
Contributor

zah commented Jun 17, 2022

Interesting. Perhaps the name of the function should then reveal that a big endian number is being loaded. The name hexToPaddedByteArray is suggesting that the function is dealing with bytes and it would be surprising to me that some of the bytes could end up being ignored. In other words, it might be better to define a new function with a more descriptive name.

@etan-k
Copy link
Author

etan-k commented Jun 17, 2022

Well, hexToPaddedByteArray already assumes big endian, hence "padded" in its name. It extends shorter hex-strings with leading zeroes. It just doesn't cut extra zeroes from the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants