Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore leading zeroes in hexToPaddedByteArray #86

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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